Conversation
npm-packlist emits strict un-ignorable rules for package.json's "main" and string-form "browser" fields, so those files land in the tarball even when a "files" allowlist omits them or .npmignore matches them. bun pm pack already did this for "bin" targets but not for "main" or "browser", so a package whose files list forgot its own entry point would ship a tarball that resolves main to a missing file. Mirror the existing bin handling: extract the two entry point paths, queue them as optional items (a missing file is not an error), and dedupe against the tree walk so a main that is also matched by files or not ignored appears exactly once.
|
Updated 9:20 PM PT - Jul 28th, 2026
❌ @robobun, your commit 5f018c6 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36266That installs a local version of the PR into your bun-36266 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Walkthrough
ChangesPackage entry-point packing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Confirmed this fixes #15602. That report's and npm's output there is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
When main/browser/bin names a directory, openat succeeds on POSIX so the ENOENT guard is bypassed and read() later fails with EISDIR. Check S_ISREG after fstat and skip optional items that are not regular files. npm does not force-include a directory main either, so skipping matches npm here. Also fixes the pre-existing crash for string bin targets that name a directory.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
package.json is written out of band by archive_package_json (with workspace: protocol rewriting applied) before the drain loop. Both tree walks already skip it at depth 1 for that reason; the force-queued bin and main/browser paths bypass the tree walks, so a main/browser/bin value of "./package.json" produced a duplicate tarball entry (the second being the raw on-disk bytes, overwriting the edited copy on extraction). Filter package.json out when collecting bins and entry points.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/cli/pack_command.rs`:
- Around line 2747-2756: Update the optional-path handling in the pack command
so non-regular paths are rejected before the blocking or side-effectful openat
operation; use a pre-open metadata/type check, or an explicitly safe nonblocking
open-and-validate sequence that prevents FIFOs from blocking and device nodes
from being opened. Preserve the existing stats decrement, progress completion,
and continue behavior for skipped optional paths.
- Around line 2343-2347: Update the entry-point handling around PackQueueItem
creation so optional entries suppress only ENOENT; propagate permission, I/O,
malformed-path, and all other filesystem errors. Preserve successful packing and
the missing-file behavior for entries marked optional, using the existing
PackQueueItem/PackQueue flow and error classification mechanisms.
- Around line 2343-2347: Update the entry-point queuing flow around
PackQueueItem and the later package-file open operation to resolve paths beneath
root_dir without following symlinks, including symlinked parent components. Use
the platform’s no-follow or component-safe openat resolution flags/API, while
preserving regular-file validation and existing PackQueueItem behavior.
In `@test/cli/install/bun-pack.test.ts`:
- Around line 1632-1751: Extend the entry-point edge-case tests around the
existing main-focused cases to also cover string-form browser entries.
Parameterize the missing-file, directory, deduplication, and ignore-rule
scenarios over main and browser, or add equivalent browser tests, while
preserving the current assertions for each generated tarball.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a69a179d-4065-4aab-b449-0c112c92bec4
📒 Files selected for processing (2)
src/runtime/cli/pack_command.rstest/cli/install/bun-pack.test.ts
There was a problem hiding this comment.
LGTM — both issues from earlier passes (directory main crash, package.json duplicate) are fixed with tests, and this run's bug hunt found nothing new.
What was reviewed:
get_package_entry_pointsmirrorsget_package_binsexactly (samenormalize_buf::<Posix>,bin_path_escapes_root,package.jsonfilter).is_package_entry_pointdedup wired into all three tree walks at the same site as the bin-file dedup;entry_subpathuses/on all platforms so the comparison holds on Windows.- The
!ISREGskip is applied in both the real drain and the dry-run drain, and mirrors the pre-existingoptionalopen-error handling for bins. - Entry points inside a
directories.bintree still pick up+xviais_package_bininadd_archive_entry.
Extended reasoning...
Overview
Adds npm-packlist-compatible force-inclusion of "main" and string-form "browser" to bun pm pack, alongside the existing "bin" handling. Touches src/runtime/cli/pack_command.rs (~94 lines: new get_package_entry_points / is_package_entry_point, three tree-walk dedup call sites, force-queue loop, !ISREG skip in both drain loops, and a package.json filter added to get_package_bins) and test/cli/install/bun-pack.test.ts (7 new tests in an entry points describe block).
Security risks
None material. Entry-point paths come from the package's own package.json, are normalized with the same resolve_path::normalize_buf::<Posix> used for bins, and are rejected by bin_path_escapes_root if absolute or ..-escaping. openat is relative to the package root fd. Symlink-following and swallow-all-errors-when-optional are pre-existing bin semantics reused here, not new surface.
Level of scrutiny
Medium. This is a user-facing CLI behavior change affecting published tarball contents, but it is a strict superset of prior output (adds files that npm already includes), closely mirrors the established bin path, and is verified against npm-packlist semantics. It went through two prior review rounds where I flagged a directory-main crash regression and a package.json duplication — both were fixed (c897db0, c2ee42d) with regression tests. CodeRabbit's remaining points (ENOENT-only suppression, symlink traversal, FIFO-before-open) were correctly declined as pre-existing bin-drain semantics out of scope for this npm-compat change.
Other factors
- Full
bun-pack.test.tssuite (81 tests) passes on debug+ASAN and release; the new tests fail on main without the fix per the PR's evidence block. - Test coverage is thorough:
filesallowlist,.npmignore, dedup withfiles, missing file,package.jsonself-reference, directory-valuedmain, object-formbrowser, and the no-+xinvariant. - All inline review threads are resolved. No human reviewer has weighed in with unaddressed concerns.
- I confirmed
entry_subpathbuilds paths with/on every platform, so thestrings::eql_longdedup against Posix-normalized entry points is sound on Windows.
|
CI status: the diff is green. Remaining red on #84482 is unrelated to
The Ready for review. |
… pack output (#40959) ### Problem - `test/cli/install/bun-pack.test.ts` takes 10.7s on debian 13 x64-asan in the serial phase (build 108487). Its 80 tests run one at a time, each with one to five `bun pm pack` spawns. - The assertions are loose: the harness `pack()` helper only checks that stderr lacks `error:`, `warning:`, `failed` and `panic:`, tarballs are checked with `toMatchObject`, and the `--filename="out/foo.tgz"` error case accepts any outcome. ### Fix - Each test builds its tree with `tempDir` instead of the shared `beforeEach` directory. The describes are `describe.concurrent`, the top-level tests `test.concurrent`. - A local `runPack()` returns stdout and stderr, raw and normalized with `normalizeBunSnapshot`. The normalized stdout masks the shasum, the integrity and the packed size, which depend on the compressor. - Every test asserts that `err` is `""` (or the exact `$ script` echo), the exact stdout, the exit code, and the full entry list with `toEqual`. Error cases assert the exact message and that nothing was written. - Verified: local debug+ASAN build, 80 tests in 20.6s and 21.8s before, 83 tests in 6.9s, 6.9s and 7.0s after. `--rerun-each=3` passes 249 of 249. CI debian 13 x64-asan: 10.7s before, 3.0s after (build 108529). ### Background - `describe.concurrent` runs a group's async tests up to `--max-concurrency` at a time (20, or 5 in ASAN builds). Groups and top-level `test.concurrent` tests overlap, so a shared module-level directory is not safe. - `toMatchInlineSnapshot` works in concurrent tests, but one call site cannot hold different values across `test.each` rows. The tables compare a line array instead. <details><summary>Notes</summary> - Test count 80 to 83: `--gzip` is split into three rejected-level cases and one level 0 vs level 9 case, and the `--filename="out/foo.tgz"` error row is its own test. No test was removed or skipped. - `readTarball` from `bun:internal-for-testing` parses a tarball into its entries, shasum and integrity. - Lines that use `expect.stringMatching` instead of an exact value: the package.json size and the unpacked size in the tables whose rows change package.json (scoped names, `workspace:` specs, `bundledDependencies` spelling), and in the two lifecycle tests whose scripts embed `bunExe()`, so the size depends on the path of the bun binary. On the darwin CI agent that path pushes package.json past 512 bytes and the size prints as `0.58KB`, so those two matchers accept any size format (build 108529 caught the `NNNB`-only version). - The exact output records some current behavior as-is: the name `//` writes `-1.1.1.tgz` but prints `//-1.1.1.tgz`; the name `@//` fails with `failed to open tarball file destination: ".../-/-1.1.1.tgz"` (the old test only asserted a non-zero exit); transitive scoped bundled deps print without their scope (`bundled dep3` for `@scoped/dep3`); `--dry-run` prints the on-disk package.json size while a real pack prints the re-serialized size; empty files print as `0KB`. None of these is changed here. - `bun install` still runs once per `workspace:` lockfile case (7 runs). They are workspace-only and contact no registry. The `bundledDependencies` tests already built `node_modules` on disk. - The release binary runs the file in 0.19s locally. Under ASAN each spawned pack still costs 150 to 400ms, so what remains is CPU bound: about 85 debug `bun pm pack` runs, 5 at a time. - Open PRs that add cases to this file (#36266, #38715, #36699, #38813, #38721, #38739, #38835, #38720, #38749, #38784, #38707, #38716) need a rebase onto the new shape: a `tempDir` tree plus `runPack(dir)`. - CI durations before, build 108487 serial phase: 10.7s debian 13 x64-asan, 2.0s windows 11 aarch64, 1.5 to 1.7s alpine, about 1s on the other release lanes. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-pack.test.ts <!-- robobun:evidence:end -->
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-28, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#15602) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
What
bun pm packnow force-includes the files referenced bypackage.json's"main"and string-form"browser"fields, matchingnpm pack. Previously only"bin"targets were protected, so a"files"allowlist that omitted the entry point (or an.npmignorethat matched it) produced a tarball whose"main"pointed at a file that was not shipped.Repro
Before: tarball contains only
package/package.jsonandpackage/dist/bundle.js.lib/index.jsandlib/browser.jsare missing, sorequire("mainless")fails after install.After: tarball also contains
package/lib/index.jsandpackage/lib/browser.js, same asnpm pack.The
.npmignorevariant behaves the same way:"bin"was kept but"main"was dropped.Cause
is_unconditionally_included_fileinsrc/runtime/cli/pack_command.rscoverspackage.json,LICENSE*,LICENCE*, andREADME*;bintargets are protected separately viaget_package_bins/is_package_bin. There was no rule for"main"or"browser". npm-packlist'sprocessPackageappends strict!/${main}/!/${browser}rules alongside thebinrules.Fix
Add
get_package_entry_points, which extracts"main"and the string form of"browser"(the object-map form is a remap table, not a file path), normalizes each path, and filters out values that duplicate a"bin"target or each other. These are queued asoptional: true(a missing file is not an error, matching bin handling) and deduped against the three tree walks so an entry point that is also matched by"files"or not ignored appears exactly once. Entry points do not pick up the executable bit thatbintargets get.Verification
New
entry pointsdescribe block intest/cli/install/bun-pack.test.ts:"main"+"browser"are included when"files"omits them"main"is included when.npmignorematches it, and does not gain+x"main"already inside the"files"tree is not duplicated"main"that does not exist on disk is not an error"browser"is not force-included (npm compat)Full
bun-pack.test.tssuite passes (81 tests).Fixes #15602
[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 1
evidence per changed file