fix(pack): always include bin even if not included by files
#23606
Conversation
|
Updated 4:28 PM PT - Oct 14th, 2025
❌ @dylan-conway, your commit 090dbc2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 23606That installs a local version of the PR into your bun-23606 --bun |
WalkthroughRefactors pack queue to use PackQueueItem with zero-terminated paths, updates traversal and packing functions to accept bins and PackQueue, and adjusts path handling throughout. Adds tests verifying bin and directories inclusion and deduplication. Updates an internal ban-limit for std.fs.Dir. Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (9)test/**📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
**/*.zig📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/**/*.zig📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/cli/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/cli/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
test/**/*.{ts,tsx,js,jsx,mjs,cjs}📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
🧠 Learnings (5)📚 Learning: 2025-09-26T01:10:04.233ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-10-12T02:22:34.373ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
🧬 Code graph analysis (1)test/cli/install/bun-pack.test.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (15)
Comment |
WalkthroughRefactors pack logic to use PackQueueItem in a priority queue, updates comparison to sort by item.path, threads bins and pack_queue through traversal functions with signature and return-type changes, adjusts path handling and dedupe, adds tests for bin inclusion and directories/files interaction, and updates an internal ban-limits value. Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/cli/pack_command.zig(24 hunks)test/cli/install/bun-pack.test.ts(1 hunks)test/internal/ban-limits.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/internal/ban-limits.jsontest/cli/install/bun-pack.test.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/cli/pack_command.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/cli/pack_command.zig
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/install/bun-pack.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/install/bun-pack.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/install/bun-pack.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/install/bun-pack.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/cli/install/bun-pack.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/cli/install/bun-pack.test.ts
🧠 Learnings (3)
📚 Learning: 2025-09-26T01:10:04.233Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: test/internal/ban-limits.json:25-26
Timestamp: 2025-09-26T01:10:04.233Z
Learning: The test/internal/ban-limits.json file contains accurate counts that are validated by tests - if the counts were incorrect, the associated tests would fail, so the values in this file reflect the actual current state of the codebase.
Applied to files:
test/internal/ban-limits.json
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/cli/install/bun-pack.test.ts
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/cli/install/bun-pack.test.ts
🧬 Code graph analysis (1)
test/cli/install/bun-pack.test.ts (2)
test/harness.ts (2)
pack(1304-1326)bunEnv(49-64)src/js/internal-for-testing.ts (1)
readTarball(125-125)
🔇 Additional comments (8)
test/internal/ban-limits.json (1)
36-36: Count update looks correctThe adjusted ban-limits count aligns with the test-driven source of truth for this file.
Based on learnings
src/cli/pack_command.zig (6)
217-228: Solid queue refactor and orderingSwitching to PackQueueItem and ordering by path is clean and future-proof. It enables optional entries and stable, predictable output.
236-244: Correctly integrates bins with “files” traversal and dedupe
- Passing bins and pack_queue into iterateIncludedProjectTree/addEntireTree prevents duplicates.
- Skipping bin dirs/files during include traversal aligns with pre-seeding/explicit bin handling.
- Dedupe map avoids repeated file enqueue within “files”-driven traversal.
Also applies to: 329-334, 343-347, 356-363, 371-380
385-392: addEntireTree bin-aware and dedupedDedupe + bin-file/dir skipping avoids double-inclusion while still walking included subtrees.
Also applies to: 470-478, 480-485
803-811: Default traversal now bin-awareSkipping bin files/dirs in iterateProjectTree prevents duplication with the bin prepass. Good separation of concerns.
Also applies to: 819-820, 850-851, 885-891, 893-899
967-975: Bin parsing and path handling are robust
- Normalizing to posix paths and storing as [:0]const u8 is appropriate.
- Memory management via dupeZ and per-bin free at scope end is correct.
Also applies to: 985-992, 1001-1006, 1019-1026
1380-1401: Bin inclusion semantics match npm; optional flag is a nice touch
- Pre-adding file bins (optional=true) ensures inclusion even when “files” omits them.
- directories.bin traversal includes nested files while exec bits only apply to direct children (see isPackageBin), matching expectations.
- Integrating bins into both “files” and default traversal via skip rules avoids duplicates.
Consider adding a test for a missing bin file (bin points to non-existent path) to confirm optional behavior matches npm for both normal and --dry-run modes. I can add it if you like.
Also applies to: 1433-1442, 1449-1457
test/cli/install/bun-pack.test.ts (1)
1284-1314: Great coverage for bins × files/directories and dedupe
- Verifies bins are included even when “files” omits them.
- Confirms directories.bin is traversed and coexists with “files”.
- Ensures deduplication when entries appear via both routes.
Consider adding:
- A case where bin points to a missing file to validate optional handling and Total files in --dry-run.
- An assertion that nested entries under directories.bin (e.g. bins/what/what.js) do not get exec bits, while direct children do. Current behavior enforces this.
Also applies to: 1316-1351, 1353-1388
| while (pack_list.removeOrNull()) |item| { | ||
| const stat = root_dir.statat(item.path).unwrap() catch |err| { | ||
| if (item.optional) { | ||
| continue; | ||
| } | ||
| Output.err(err, "failed to stat file: \"{s}\"", .{item.path}); | ||
| Global.crash(); | ||
| }; | ||
|
|
||
| ctx.stats.unpacked_size += @intCast(stat.size); | ||
|
|
||
| Output.prettyln(packed_fmt, .{ | ||
| bun.fmt.size(stat.size, .{ .space_between_number_and_unit = false }), | ||
| filename, | ||
| item.path, | ||
| }); |
There was a problem hiding this comment.
Dry-run “Total files” can be inflated when skipping missing optional entries
In dry-run, missing optional items are silently skipped in listing but not decremented from stats.total_files, unlike the non-dry-run path where you decrement when opening fails. Adjust stats in the dry-run branch to keep summary accurate.
Apply this diff:
- while (pack_list.removeOrNull()) |item| {
- const stat = root_dir.statat(item.path).unwrap() catch |err| {
- if (item.optional) {
- continue;
- }
+ while (pack_list.removeOrNull()) |item| {
+ const stat = root_dir.statat(item.path).unwrap() catch |err| {
+ if (item.optional) {
+ // keep Total files accurate in --dry-run
+ ctx.stats.total_files -= 1;
+ continue;
+ }
Output.err(err, "failed to stat file: \"{s}\"", .{item.path});
Global.crash();
};📝 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.
| while (pack_list.removeOrNull()) |item| { | |
| const stat = root_dir.statat(item.path).unwrap() catch |err| { | |
| if (item.optional) { | |
| continue; | |
| } | |
| Output.err(err, "failed to stat file: \"{s}\"", .{item.path}); | |
| Global.crash(); | |
| }; | |
| ctx.stats.unpacked_size += @intCast(stat.size); | |
| Output.prettyln(packed_fmt, .{ | |
| bun.fmt.size(stat.size, .{ .space_between_number_and_unit = false }), | |
| filename, | |
| item.path, | |
| }); | |
| while (pack_list.removeOrNull()) |item| { | |
| const stat = root_dir.statat(item.path).unwrap() catch |err| { | |
| if (item.optional) { | |
| // keep Total files accurate in --dry-run | |
| ctx.stats.total_files -= 1; | |
| continue; | |
| } | |
| Output.err(err, "failed to stat file: \"{s}\"", .{item.path}); | |
| Global.crash(); | |
| }; | |
| ctx.stats.unpacked_size += @intCast(stat.size); | |
| Output.prettyln(packed_fmt, .{ | |
| bun.fmt.size(stat.size, .{ .space_between_number_and_unit = false }), | |
| item.path, | |
| }); |
🤖 Prompt for AI Agents
In src/cli/pack_command.zig around lines 2484 to 2498, when stat() fails for an
optional item the code currently continues without adjusting summary counts,
causing dry-run "Total files" to be inflated; modify the error handling in that
catch so that if item.optional you decrement ctx.stats.total_files (matching the
non-dry-run path) before continue, and leave non-optional behavior unchanged
(log error and Global.crash()).
|
LGTM once windows tests are fixed |
3 participants
What does this PR do?
Fixes #23521
How did you verify your code works?
Added 3 previously failing tests for
"bin","directories.bin", and deduplicating entry in both"bin.directories"and"files"