Repository navigation
Conversation
…in sideEffects globs and --filter paths
A "sideEffects" glob is joined onto the package directory, and a
`--filter` path selector onto the directory the command runs in. The
joined string was matched as one glob, so a `[` or `{` in a directory
name was glob syntax and the directory did not match itself.
`bun build` then dropped a file that the list names, and
`--filter ./packages/a` found no workspace.
`bun_glob::literal_base_len` finds where the joined string stops being
the directory. Both callers compare that part as bytes
(`bun_glob::strip_base`, which takes `/` and `\` as one separator on
Windows) and give only the rest to the matcher.
A path selector that spells a workspace directory byte for byte
selects that directory, beside what the selector matches as a glob.
The "sideEffects" lookup no longer copies the path for each call. A
name selector does not make `--filter` resolve the working directory.
`bun_glob` joins the crates that `bun run rust:miri` runs, so CI runs
its two unit tests.
…mes have glob characters
- `bun build` keeps a file that a "sideEffects" glob lists when the
package directory, or a directory above it, is named `[client]`,
`{old,new}`, `a[b` or `a{b`, for the package itself, a dependency,
a mixed list, entries that go above the package with `..`, and a
"main" outside the package.
- `--filter` path selectors find their workspaces under `[ws]` and
`{a,b}`, and select a workspace directory named `[x]` from inside it
and from other directories, with and without a directory named `x`
beside it, for `bun run`, `bun add` and `bun remove`.
- Three cases pin answers that do not change: eleven "sideEffects"
spellings and a "main" outside the package in a plain directory, and
an absolute import specifier.
|
Status
|
|
Updated 8:03 AM PT - Oct 24th, 2026
✅ @robobun, your commit 851c4dcaa4463637e3aa9437df309c75b42ca2e0 passed in 🧪 To try this PR locally: bunx bun-pr 44939That installs a local version of the PR into your bun-44939 --bun |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 6 minutes. View limit details
No actionable comments were generated in the recent review. ℹ️ Recent review info
WalkthroughGlob matching now treats directory prefixes as literal paths and matches patterns against the remaining path. Workspace selectors and package side-effects checks use this behavior, with added tests for paths containing glob syntax. ChangesLiteral-base glob matching
Suggested reviewers: Priority: Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly describes the main change: directory names are no longer interpreted as glob syntax for sideEffects globs or --filter paths. |
| Description check | |
The description explains the problem, fix, scope, known limitations, performance impact, and extensive verification results. It does not use the template headings exactly, but it provides the required… |
Full details: Description check
-
Explanation
The description explains the problem, fix, scope, known limitations, performance impact, and extensive verification results. It does not use the template headings exactly, but it provides the required information through the Problem, Fix, and Tests sections.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the Base::Subtree ancestor walk in workspace_selection.rs: the new base.len() + i > 0 stop reduces to main's i > 0 when the selector climbed to / (empty base) and otherwise stops at the base directory itself, which main also tested as a glob candidate, so the selected set is unchanged for plain directories. The base_len as u32 cast in SideEffectsGlob::init is bounded by the package.json path length, and the literal_base_len unit cases cover root, empty, and ..-climbing inputs.
Extended reasoning...
The change replaces joined-glob matching with a literal-base-plus-glob-tail split in two sites (sideEffects globs in the resolver and path selectors in --filter), adds three helpers to bun_glob with unit tests, and extends three test files. It touches no auth, crypto, or network surface. One confirmed finding (candidate path no longer slash-normalized on POSIX) is posted inline; the ancestor-walk and index-arithmetic concerns I checked were equivalent to main.
The glob matcher on main is new: patterns are prepared once (bun_glob::Prepared) and have more syntax. The tail of a sideEffects glob and of a path selector is now a prepared pattern, and the match under a base is Prepared::match_under.
The matcher on main now reads an unclosed `[` or `{` as itself, so
`a[b` and `a{b` match on main too. `{1..3}` and `@(client)` are new
syntax there and do not match themselves.
There was a problem hiding this comment.
I reviewed the updated revision and didn't find any bugs. Because it changes how every sideEffects glob and --filter path selector is matched, a human look would still be worthwhile.
What was reviewed:
literal_base_lencuts only at a component boundary of both strings, so the tail handed tomatch_underis always empty or starts with a separator (thedebug_assertcannot be reached by user input), and sibling directories (/t/appvs/t/app2) are rejected because that leading/must match a separator.package_json.rs:self.source.pathis the same path the entry was cut against at parse time (PathName::diris a prefix oftext, including the drive-letter case), and on Windows the\candidate path matches the/-normalized tail through the matcher's WINDOWS mode.workspace_selection.rsSubtree climb stops at the base, or at the root when the base is empty, matching the oldi > 0guard; otherPrepared::new/glob::r#matchcallers (pack, test, archive, update_scope, negated workspaces) match names or relative globs, not a directory-joined pattern.
Extended reasoning...
The change adds three primitives to the bun_glob crate and reworks how package.json "sideEffects" globs and --filter path selectors are stored and matched, replacing whole-string glob matching of a joined path with a literal directory-prefix compare plus a glob tail; it also moves has_side_effects onto PackageJSON and updates the resolver caller. It touches no auth, crypto, or injection surface, but it alters tree-shaking decisions for every package with glob sideEffects and the selection semantics of --filter, including a new rule that a selector spelling a workspace directory selects it. The bug hunt ran dry and the new commits since the prior review (merge of main plus a test adjustment) did not change the core logic, but the breadth of affected behavior and the untested Windows separator path make a human look worthwhile rather than approval.
…-literal-base # Conflicts: # src/install/PackageManager/workspace_selection.rs # src/resolver/package_json.rs
…nd --filter paths
Since 1.4.4 the matcher reads ?( ), *( ), +( ), @( ) and !( ) as groups in every
pattern, so a directory with such a name hits the same fault as [client] and
{old,new}. The unit test has one pair of rows for each of the five groups.
Every test in bun-add-filter.test.ts runs concurrently, and the c++(old) row adds three more installs. On a debug build, tests next to it then go past the 5 s limit in 3 of 4 runs of the file, and in 0 of 4 runs without the row. filter-workspace.test.ts and dce.test.ts keep the c++(old) and @(scope) cases, and the selection code is the same for bun run and bun add.
The cases next to them compare the whole stderr, and the run helper removes the progress lines for that purpose. A warning or any other line now fails the case, and a failure shows the text that the command printed.
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; the exact-stderr assertions in bun-add-filter.test.ts now match their siblings. Because this changes what bun build tree-shakes and what --filter selects (including the deliberate choice that ./packages/[x] selects both [x] and x), a human look at that design call is still worthwhile.
What was reviewed:
literal_base_len/strip_baseconsistency between parse time (json_source.path.name().dir) and match time (self.source.path.text[..base_len]): same path text is copied intopackage_json.source, so the prefix is identical.- Windows separators: the tail is posix-normalized and
match_prepared::<{cfg!(windows)}>treats\as a separator in the candidate, so droppingnormalize_path_for_globon the candidate is equivalent there. Base::Subtreewalk-up bound (base.len() + i > 0) reaches the base itself but not/, matching main'si > 0for a plain cwd; root-cwd{.}still misses children, as on main.
Extended reasoning...
The diff adds literal_base_len, strip_base and Prepared::match_under to src/glob/lib.rs with a cfg(test) module, and uses them in src/resolver/package_json.rs (sideEffects globs stored as a literal base length plus a glob tail, has_side_effects moved onto PackageJSON) and src/install/PackageManager/workspace_selection.rs (path and subtree selectors keyed on the cwd prefix), plus tests in dce.test.ts, bun-add-filter.test.ts and filter-workspace.test.ts. It touches no auth, crypto or injection surface; the untrusted inputs are package.json strings and CLI args, and all slicing is bounded by literal_base_len which never exceeds either input. Deferring rather than approving because the change alters bundler DCE and workspace selection semantics and includes a user-visible design decision (a bracketed selector also matches the unbracketed sibling) that a maintainer should weigh; the only prior inline nit was addressed by the newest commit.
Problem
[client]/appor{old,new}/app,bun builddrops a file that a"sideEffects"glob lists, at exit 0.--filter ./packages/aprintserror: No workspace packages matched the filter "./packages/a".--filter '!./packages/a'excludes nothing.c++(old)/appfails since 1.4.4: Addbun lintandbun format#44805 made+(old)a group in every pattern.PackageJSON::parse(src/resolver/package_json.rs) andworkspace_selection::parse(src/install/PackageManager/workspace_selection.rs) join a directory in front of the pattern. They match the whole string as a glob, so[client]is a character class.Fix
bun_glob::literal_base_lenfinds where the joined string stops being the directory. Callers compare that part as bytes and match only the rest.dce.test.ts,filter-workspace.test.ts,bun-add-filter.test.ts. 64 of 67 new cases fail on main. Plain directories: no change (Notes).Background
"sideEffects"lists the files a bundler must keep when nothing uses their exports.--filterthat starts with.or is{dir}selects by directory.../entries).Downsides
./src/[id].jsdoes not keep[id].js,--filter './packages/[x]'also selectsx.selectruns 2,640 -> 3,064 instructions for a path selector and 989 -> 1,016 for a name. With 200 both go down (Notes).fs.globexclude arrays,bun ./index.htmlunder{a,b}/, resolver: match sideEffects entries without "/" in every directory (names in the exact set, globs as **/) #36119 (Notes).Notes
The rule. The joined string is cut at the longest prefix that it shares with the directory, at a path component boundary of both. So an entry with
..has a shorter directory part (../shared/ind*.jsis under the parent of the package), and text that goes back into the directory it left is a name again (../[x]typed insidepackages/[x]). What is left after the cut is empty or starts with a separator, so it reads as it did inside the joined string.Who is affected. A project whose path, or a workspace or package directory in it, has a name with a closed bracket or brace group (
[client],{old,new},{1..3}). Since 1.4.4 also a name with a group:c++(old),app@(v2),+(app),*(x),?(x). On main the matcher already reads an unclosed[or{as itself. Only a linked bundle loses the file (bun build,Bun.build, production HTML routes). Running the source keeps it. For--filter, a positive path selector fails loud. A negated one fails silent:bun addandbun removethen edit the workspace that the selector names. I searched the tracker for bracket and brace directories withsideEffectsand with--filterand found no report. The nearest are #30320 (Windows separators) and #40650 (a list read asfalse).The same project in 13 folders. A package in
node_moduleshas"sideEffects": ["./polyfill.js", "./src/*.setup.js"], and a workspace root haspackages/aandpackages/b. Release builds of 1.4.3 (c6da4a4), 1.4.4 (38afee6) and main (canary a8fcd31), and a debug build of this PR. esbuild 0.21.5 keeps the file in every folder.app,app (copy),app(1)app[1],[tenant],app{a,b}+(app),c++(old),app@(v2),@(scope),*(app),?(app)!(app)"Wrong" is all of these together:
bun builddropssrc/a.setup.jsat exit 0,bun run --filter ./packages/aand--filter '{packages}'exit 1 with no match, and--filter '!./packages/a'runs both workspaces.!(app)is right on main because the name itself matches "notapp". The groups enteredsrc/globwith #44805 and #45043, which are in 1.4.4 and not in 1.4.3.Measured. Release builds of the earlier merge base 5866b5f and of this PR at that base, same toolchain. The numbers were not measured again after the merge of main at 5e18294. In these functions that merge only replaces the joins at parse time with length-checked joins, on both sides. An instruction count is a gdb
stepicount on one thread, from the call of the named function to its return, with everything that it calls. A call count is a gdb breakpoint hit count.primary_side_effectscalls 401 -> 401.malloc,memcpyandfreecalls in it: 400 each -> 0. The PR has no allocator call site in the lookup.primary_side_effects), for the last two lookups of 200Bun.resolveSynccalls. The two are equal. The package directory has D bytes and the list has G globs:sideEffectsfieldfalsemalloc(17 instructions),memmove(20 at D=180) andoperator delete[](23). The PR runs onememcmp(44 at D=180, 15 at D=60). The table includes them.finalize_resultfor one import from a package without asideEffectsfield: 1,219 -> 1,216 instructions.-one-insn-per-tb, two runs differ by less than 200).main.jsandmain.cssare byte-identical.size_of::<SideEffects>(): 80 -> 80 bytes.size_of::<PackageJSON>(): 656 -> 656 bytes. A stored glob is 48 bytes (was 40). Its pattern text no longer holds the directory, so it is shorter by the length of the directory.size).primary_side_effects: 5,446 -> 1,378 bytes plus 1,237 for the glob loop (nm -S).--filter, instructions inworkspace_selection::select:p0,p7)./packages/p0,./packages/p7{packages}!./packages/p7over 200 workspaces: 164,428 -> 95,838. Only the first path selector of a command joins the working directory.Plain directories. With release builds of both sides at the earlier merge base: 402 builds (67 glob entries in 6 plain directory names, 7 files each) keep the same files, and 2,670
bun run --filterruns (89 path selectors, 5 working directories, 6 plain directory names) select the same workspaces. Under 11 bracket, brace and extglob directory names the PR build gives the answer of the plain directory in all 737 builds and 4,895 runs. One more check is not in the tree: a script compared the joined glob with the split form throughBun.Globfor 1,000,000 random (plain directory, glob, path) rounds. The two forms gave the same answer in every round, and 270,404 rounds were matches.Tests. 67 new cases. 16 are in
dce.test.ts: 13 fail on main, and 3 are pins that pass there (the spellings in a plain directory, themainoutside the package in a plain directory, and the absolute specifier). 48 are infilter-workspace.test.tsand 3 inbun-add-filter.test.ts, and all of those fail on main. The directories above the project are named[client],{old,new},{1..3},@(client),c++(old),[ws],{a,b}and@(scope). Fail-before ran on the canary release build of main at a8fcd31. Debug build after the merge of main:dce.test.ts103 pass,filter-workspace.test.ts179 pass,bun-add-filter.test.ts147 pass,bundler_barrel.test.ts70 pass,multi-run.test.ts132 pass, and the three oversizedsideEffectscases ofresolve.test.tspass.bun-add-filter.test.tshas the cases for[ws]and{a,b}only. With ac++(old)case too, tests next to it reached the 5 s limit in 3 of 4 runs of the file on the debug build, and in 0 of 4 runs without it.bun-pm-licenses.test.tsandbun-prune.test.ts: 1 to 3 tests per run reach the 5 s limit on the debug build, different ones each run, and each passes when it runs alone.bun run rust:miri -p bun_glob: 2 tests pass, with one pair of rows for each of the five groups. A plaincargo test -p bun_globdoes not link on main, so the unit tests run under Miri only. Nothing was run on Windows by hand: the separator handling instrip_baseand the absolute-specifier case come from reading the resolver. The Windows lanes in CI run the three test files.*and?cannot be in a Windows file name, so*(x)and?(x)are in the unit test only. One claim in the self-review (bun check <dir>) did not reproduce.Not changed.
fs.glob,fs.globSyncandfs.promises.globwith anexcludearray exclude nothing under acwdlikeroutes/[slug]. Node v26.3.0 prints the same rows, and Node's main branch still resolves each exclude pattern against the root. A change there makes Bun differ from Node, so it is a separate decision.bun ./index.htmlunder{a,b}/fails withNo HTML files found matching. That is a different mechanism (src/js/internal/html.tstakes the resolved argument for a glob). Do not glob an HTML entry path that names a file the glob cannot match #45060 has that change.\is a path separator in asideEffectsentry and in a path selector, as on main, so it does not escape.[[]id]is the spelling of a literal bracket.sideEffectsentry matches, before and after (the directory is then./)./at any depth) edits the same function inpackage_json.rs, and test(filter-workspace): run the cases concurrently and assert the exact output #41033 edits the same test file. Whichever lands second needs a rebase.finalize_resultstill asks the same package.json twice for one path. Asking once halves the calls. That is separate work.\in a file or directory name does not work on main or here. The resolver takes it for a separator, and an import of such a file fails withCould not resolve.bun b.jsin a directory named?(app)fails withModule not foundon 1.4.3, on main and here. That is not a glob: it is the?in the path of the entry file (bun testfails while searching for test files if path contains '?' #7928, Decide in_resolvewhether a?is part of a path or starts a query #36627). The table above ran that bundle with Node.bun lintjoins a directory onto the patterns of some rule options. Withimport/no-restricted-pathszones it reports nothing under[proj],{a,b},c++(old)orapp@(v2), at exit 0. That is separate work insrc/lint.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/filter-workspace.test.ts, test/cli/install/bun-add-filter.test.ts