Conversation
…d directory
With an .npmignore like:
*
!dist/**
bun pm pack pruned dist/ at the directory level (because * matches it)
before !dist/** ever had a chance to re-include the files inside. npm
(via ignore-walk) handles this by doing a partial match against negated
rules when deciding whether to descend into a directory.
is_excluded now does the same: when evaluating a directory entry, a
negated pattern that doesn't match the directory itself but could match
a path beneath it (segment-wise prefix match) keeps the directory in the
walk. Files inside are still filtered individually, so only paths that a
negation actually matches end up in the tarball.
Fixes #15602
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesBun pack now traverses excluded directories when negated npmignore negation handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:59 AM PT - Aug 1st, 2026
⏳ @robobun, your commit 9ce36db is still building in
|
|
Reproduced with All 86 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix that keeps the pack walk from pruning directories a later !pattern reaches into.
What was reviewed:
- Traced
partial_path_matchsegment-by-segment against each of the ten test fixtures (!dist/**,!dist/*,!dist/lib/foo.js,!*/keep.js,!**/keep.js, and the re-exclusion case*/!dist/**/dist/sub); all produce the npm-verified expected sets. - Confirmed the new branch is gated on
is_dir && ignoredand only consultsrelwhen the pattern hasREL_PATH, soignore.has_rel_pathguaranteesrelwas trimmed to the ignore file's depth. - Checked that
node_modulesis skipped before pattern matching (lines ~507/737), so!**/…returning true fromLEADING_DOUBLESTAR_SLASHcannot cause a walk into it. - The
files-array include path (~536-580) uses opt-in semantics and is not affected.
Extended reasoning...
Overview
The PR fixes #15602: bun pm pack was pruning directories at the point a positive .npmignore rule (e.g. *) matched them, so a later negated rule like !dist/** never got to re-include their contents. The change adds a second un-ignore branch in is_excluded for directory entries only: if the negated pattern doesn't match the directory itself but could match something beneath it (a segment-wise prefix test mirroring minimatch's partial), the directory stays in the walk. Two small helpers (negated_pattern_matches_under_dir, partial_path_match) implement this — ~45 lines of pure slice logic, no allocation, no unsafe. Ten new parameterized tests in bun-pack.test.ts pin the packed file list for each .npmignore variant to what npm pack produces on the same fixture.
Security risks
None. This only widens directory traversal during bun pm pack in the user's own project; individual files still have to survive the per-file pattern pass to land in the tarball. node_modules and the non-overridable defaults (.git, .npmrc, lockfiles) are filtered before this code runs, so the new branch cannot re-include them. No untrusted input, no path resolution, no FFI.
Level of scrutiny
Moderate. It's a behavioral change to file-selection logic in a CLI subcommand, so the main risk is over- or under-including files relative to npm. I hand-traced the loop for every test case (including the order-sensitive * / !dist/** / dist/sub re-exclusion and the depth-limited !*/keep.js) and each matches the stated npm output. The ignored guard preserves last-rule-wins ordering, and the REL_PATH gate ensures rel is always the correctly-trimmed path when partial_path_match reads it (any REL_PATH pattern forces ignore.has_rel_path = true at parse time).
Other factors
The comment-cop bot flags on earlier commits are all resolved (comments were trimmed in 8afb04d and 9ce36db). The bug-hunting pass found nothing. The full 86-test bun-pack.test.ts suite is reported passing, and 7 of the 10 new cases fail on main — so the tests demonstrably exercise the fix. The package.json files-array path is a separate opt-in walker and doesn't share this pruning bug.
… 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 -->
This branch has not been deployed
2 participants
Fixes #15602.
Problem
With an
.npmignorelike:bun pm packprunesdist/at the directory level (because*matches it) before!dist/**ever gets a chance to re-include the files inside. npm handles this correctly via ignore-walk, which does a minimatchpartialtest against negated rules when deciding whether to descend into a directory.The same applies to
!dist/*,!dist/**/*,!dist/lib/foo.js,!*/keep.js, and!**/keep.js.Fix
is_excludednow checks, for directory entries only: if a negated pattern doesn't match the directory itself but could match a path beneath it (segment-wise prefix match, equivalent to minimatch'spartial), keep the directory in the walk. Files inside are still filtered individually, so only paths a negation actually matches end up in the tarball. Pattern order is preserved, so a later positive rule (e.g.*/!dist/**/dist/sub) still re-excludes correctly.Verification
New parameterized tests in
test/cli/install/bun-pack.test.tscover ten.npmignorevariants; each expected file list was taken fromnpm pack --dry-run(npm 11.16.0) on the same fixture. 7 of the 10 fail on current main and all 10 pass with this change. The fullbun-pack.test.tssuite (86 tests) passes.