Repository navigation
Conversation
|
Updated 12:36 AM PT - Mar 20th, 2026
❌ @alii, your commit b3fd301 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 26052That installs a local version of the PR into your bun-26052 --bun |
WalkthroughRemoved LoadResult from TextLockfile.Stringifier.saveFromBinary and migrated binary→text lockfile conversion to use a propagated resolution_buf for dependency resolution; updated numerous signatures and call sites in bun.lock serialization and added regression tests for unresolved peer dependency scenarios (issue Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/lockfile/bun.lock.zig (1)
645-686: Consider deduplicating optional peers to avoid redundant entries.If a dependency has multiple behaviors (e.g., both
devandpeer), it will be processed in multiple iterations of the outerworkspace_dependency_groupsloop. Since the check at line 678 usesdep.behavior.peer(the dependency's actual behavior) rather than the current group behavior, an unresolved dev+peer dependency would be appended tooptional_peers_bufin both thedevDependenciesandpeerDependenciesiterations.This results in duplicate entries in the
optionalPeersJSON array—valid but wasteful.💡 Optional: Check before append to avoid duplicates
// If a required peer dependency is not resolved, add it to optionalPeers // to prevent lockfile loading errors when the peer is not installed. if (dep.behavior.peer and !dep.behavior.optional and resolution_buf[dep_id] == invalid_package_id) { + // Avoid duplicates if dep has multiple behaviors + const already_added = for (optional_peers_buf.items) |existing| { + if (existing.eql(dep.name)) break true; + } else false; + if (!already_added) { try optional_peers_buf.append(dep.name); + } }
📜 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 (1)
src/install/lockfile/bun.lock.zig
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/install/lockfile/bun.lock.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@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/lockfile/bun.lock.zig
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24880
File: packages/bun-lambda/package.json:12-13
Timestamp: 2025-11-20T19:51:59.485Z
Learning: In the bun repository, explicitly adding transitive dependencies as direct dependencies in package.json is acceptable and encouraged when addressing security vulnerabilities (e.g., adding "braces": "^3.0.3" to force upgrade from vulnerable versions < 3.0.3 affected by CVE-2024-4068). This pattern ensures vulnerable transitive dependencies are upgraded even when direct dependencies haven't updated their requirements.
📚 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:
src/install/lockfile/bun.lock.zig
📚 Learning: 2025-11-20T19:51:59.485Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24880
File: packages/bun-lambda/package.json:12-13
Timestamp: 2025-11-20T19:51:59.485Z
Learning: In the bun repository, explicitly adding transitive dependencies as direct dependencies in package.json is acceptable and encouraged when addressing security vulnerabilities (e.g., adding "braces": "^3.0.3" to force upgrade from vulnerable versions < 3.0.3 affected by CVE-2024-4068). This pattern ensures vulnerable transitive dependencies are upgraded even when direct dependencies haven't updated their requirements.
Applied to files:
src/install/lockfile/bun.lock.zig
📚 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/lockfile/bun.lock.zig
🔇 Additional comments (4)
src/install/lockfile/bun.lock.zig (4)
30-37: LGTM! Clean API simplification.Removing the
load_resultparameter and deriving resolution information directly fromlockfile.buffers.resolutions.itemssimplifies the call sites and eliminates theLoadResultdependency from this module.
676-680: Core fix correctly identifies unresolved required peer dependencies.The logic correctly handles the case where a required peer dependency couldn't be resolved during install:
dep.behavior.peer- is a peer dependency!dep.behavior.optional- is required (not marked optional)resolution_buf[dep_id] == invalid_package_id- has no valid resolutionAdding these to
optionalPeersprevents the lockfile reader from erroring when it encounters a peer dependency entry without a corresponding resolution.
837-880: Consistent implementation withwritePackageInfoObject.The iteration pattern change from map-based to direct slice iteration and the unresolved peer check logic are consistent with the changes in
writePackageInfoObject. The same minor duplicate entry consideration applies here.
628-639: Function signature appropriately extended.Adding
resolution_bufas a parameter enables the unresolved peer dependency check while maintaining a clean, data-driven design that avoids the need for the previousLoadResultindirection.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/cli/install/migration/yarn-lock-migration.test.ts`:
- Around line 6-8: Remove the custom test timeout call in the beforeAll hook:
locate the beforeAll block that calls setDefaultTimeout(1000 * 60 * 5) and
delete that setDefaultTimeout invocation (or remove the entire beforeAll if it
only sets the timeout). Ensure no other tests add custom timeouts; rely on Bun's
built-in timeouts and investigate underlying test slowness if CI failures
persist.
📜 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 ignored due to path filters (1)
test/cli/install/migration/__snapshots__/yarn-lock-migration.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
test/cli/install/migration/yarn-lock-migration.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.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 usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/cli/install/migration/yarn-lock-migration.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/cli/install/migration/yarn-lock-migration.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 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/cli/install/migration/yarn-lock-migration.test.ts
🧠 Learnings (16)
📓 Common learnings
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.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
📚 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} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/cli/install/migration/yarn-lock-migration.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/install/migration/yarn-lock-migration.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/install/migration/yarn-lock-migration.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/install/migration/yarn-lock-migration.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) : Use `normalizeBunSnapshot` to normalize snapshot output of tests
Applied to files:
test/cli/install/migration/yarn-lock-migration.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/install/migration/yarn-lock-migration.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/install/migration/yarn-lock-migration.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/cli/install/migration/yarn-lock-migration.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/install/migration/yarn-lock-migration.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/install/migration/yarn-lock-migration.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/cli/install/migration/yarn-lock-migration.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/install/migration/yarn-lock-migration.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/install/migration/yarn-lock-migration.test.ts
📚 Learning: 2026-01-14T21:08:10.407Z
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.407Z
Learning: Run Node.js compatibility tests using `bun bd <file-path>` command, not `bun bd test <file-path>`
Applied to files:
test/cli/install/migration/yarn-lock-migration.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 unit tests by module in directories like `/test/js/bun/` and `/test/js/node/`.
Applied to files:
test/cli/install/migration/yarn-lock-migration.test.ts
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@robobun adopt this PR and get it mergable. |
|
✅ CI failures are unrelated to this PR:
Neither file was modified by this PR. All review threads resolved. Ready for merge. |
dbeecb9 to
d00544f
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
2b1c843 to
ce8aac4
Compare
When writing a lockfile, unresolved required peer dependencies are now added to optionalPeers. This prevents bun pm ls and other commands from failing with InvalidPackageInfo when loading a lockfile that has peer dependencies that couldn't be resolved. Uses the resolution buffer to check if a peer dep is resolved, replacing the previous pkg_map approach that only worked during binary lockfile migration. Fixes #26046
98a1847 to
8c2c28b
Compare
|
Closing this PR because it has been inactive for more than 90 days. |
Summary
Fix
bun pm lsfailing withInvalidPackageInfowhen a package has required (non-optional) peer dependencies that are not installed.Problem
When installing a package with required peer dependencies that aren't auto-installed (e.g., due to version conflicts), the lockfile was written with the peer dep in
peerDependenciesbut NOT inoptionalPeers. This caused subsequent commands likebun pm lsto fail with "Error loading lockfile: InvalidPackageInfo" because the lockfile reader expects unresolved peer deps to be inoptionalPeers.Solution
Changed the lockfile writing logic to check if a peer dependency is resolved using the resolution buffer. If a required (non-optional) peer dependency has
invalid_package_idas its resolution, it's added tooptionalPeers. This replaces the previouspkg_map-based approach which only worked during binary lockfile migration.Test plan
Regression test installs a package with a required peer dep (
>=99.0.0) that can't be satisfied by the only available version (1.0.0). Verifies:bun.lockincludesoptionalPeersfor the unresolved peerbun pm lssucceeds withoutInvalidPackageInfoFixes #26046
Verification (robobun): CI build 40149 in progress (Lint JS passed, BuildKite compiling). Diff clean, no TODO/FIXME/HACK. Core logic correctly replaces PkgMap path matching with direct resolution_buf[dep_id] == invalid_package_id check. Regression test exercises the exact failure path. Snapshot update for yarn-lock-migration correctly reflects new behavior. CodeRabbit reviews: optional dedup nit and setDefaultTimeout style note, neither blocking.