Repository navigation
Conversation
`bun run --filter`, `--workspaces`, `--parallel` and `--sequential` walk each "workspaces" entry on its own. Two entries that match the same package.json, for example "packages/*" and "packages/legacy", put the package in the list twice, so its script ran twice. `select_packages` now skips a package.json path it already has. The first match keeps its place, so the discovery order does not change.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 8 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 1:38 PM PT - Sep 18th, 2026
✅ @robobun, your commit 28628568abb058159dc51dbeebe0fba0a792b5ab passed in 🧪 To try this PR locally: bunx bun-pr 43357That installs a local version of the PR into your bun-43357 --bun |
|
Status: ready for review. How I reproduced it: I ran the script from the Notes section of the description on bun 1.4.3-canary.1 (b52d513) and on a debug build of main (367d939). The 8 new tests in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a small, self-contained dedup in select_packages with tests covering both entry points and both workspaces shapes.
What was reviewed:
- The
seen_pathscheck insrc/runtime/cli/filter_arg.rs:PackageFilterIteratorruns each walk withabsolute = true, soGlobWalker::joingoes throughbun_joinand yields a normalized path — byte-exact keying on it is sound for the./and trailing-slash spellings the tests exercise. get_or_put(...)?routesAllocErrorthroughcrate::Error::Alloc(#[from]); theStringHashMap<()>dedup idiom matchespack_command.rs.- Considered whether a workspace entry written as an absolute path would still run twice — that is base-branch behavior untouched by this diff, not a regression here.
- The new tests spawn 8 subprocesses under
describe.concurrent, drain stdout/stderr/exit concurrently, and assert a combined object;echoruns through bun's shell as in the file's existing fixtures.
Extended reasoning...
Overview
The PR touches two files. In src/runtime/cli/filter_arg.rs::select_packages it adds a StringHashMap<()> keyed on the package.json path yielded by PackageFilterIterator and continues when a path was already seen, placed before the root-skip check, the PackageJSON::parse, and the discovered.push. In test/cli/run/filter-workspace.test.ts it adds a describe.concurrent block with a fixture (root + packages/app + packages/legacy), a run() helper that counts ran-* tokens in stdout, a test.each over four overlapping workspaces shapes with --filter '*', a test.each over --workspaces, --parallel --workspaces, and --sequential --filter '*', and a test asserting the parse-failure warning is emitted once.
Security risks
None identified. The change only narrows which already-discovered paths get parsed and executed; it adds no new parsing of user data, no new I/O, and no new path construction. The map key is the walker's own output, and allocation failure propagates as an error rather than panicking.
Level of scrutiny
Low-to-moderate. The functional change is five lines with an obvious invariant. The one non-obvious dependency — that two differently-spelled workspaces entries resolve to a byte-identical key — I checked against GlobWalker::init_with_cwd (the fourth argument, absolute, is true here) and GlobWalker::join, which uses the normalizing bun_join in absolute mode. The test case ["packages/app", "packages/legacy", "./packages/legacy/"] directly exercises that path, so a normalization gap would surface as a CI failure rather than a silent miss. I could not run the suite locally (no debug build present in this checkout), so correctness of the tests rests on reading plus the author's stated run; the tests are structured so they can only pass when each package runs exactly once and the exit code is 0.
Other factors
The ruled-out candidate (absolute-path workspaces entries still running twice) is pre-existing base-branch behavior and outside what this diff claims to fix, so it does not block. The test additions follow the file's existing conventions (tempDir with using, bunExe()/bunEnv, concurrent pipe draining, combined-object assertions, echo scripts through bun's shell), cover both CLI entry points (filter_run and multi_run) and both workspaces forms, and are cheap (8 short subprocesses, concurrent). No CODEOWNERS entry covers the changed paths, the bug-hunt exit reason was dry_streak with no findings, and no third-party objections appear in the timeline.
|
I checked the absolute path case from the review above on bun 1.4.3-canary.1 (b52d513) and on this branch.
|
Problem
bun run --filter,--workspaces,--paralleland--sequentialrun a package's script twice when two entries of the"workspaces"array match it. With"workspaces": ["packages/*", "packages/legacy"],bun run --workspaces buildprintslegacy build: BUILD-legacytwice. If the script cannot run twice (mkdir out), the command exits 1.select_packages(src/runtime/cli/filter_arg.rs:181).PackageFilterIteratorstarts one glob walk per entry. Each walk drops its own repeated matches, but nothing compares two walks.select_packagespushes each path intodiscovered.Fix
select_packageskeeps a set of the package.json paths it has seen and skips a repeated path. The check runs before the parse, so a package.json that fails to parse prints its warning once.packages/legacyand./packages/legacy/give the same path). The first match keeps its place, so the discovery order does not change.bun installuses the same key (WorkspaceMap, keyed by the member path). Not in this PR: a symlink alias of a package is a different path and still runs (bun run --filterruns a package twice when a symlink that a "workspaces" glob matches points to it #43358).test/cli/run/filter-workspace.test.ts(8 new tests, all fail without the fix). Alsomulti-run.test.ts,workspaces.test.tsandrun-quote.test.ts. Self-reviewed: 3 concerns raised, 3 addressed.Background
"workspaces"in the root package.json lists the member packages, as paths or globs.select_packagesis the discovery step thatbun run --filter/--workspaces(filter_run.rs) and--parallel/--sequential(multi_run.rs) share.PackageFilterIteratorwalks<entry>/package.jsonwith oneGlobWalkerper entry, in order.Notes
I found this by reading the code. No issue reports it. The config shape exists in public bun monorepos: the root package.json of opencode lists
packages/*andpackages/slack, in the{"packages": [...]}form.Repro, offline, on 1.4.3-canary (b52d513) and main (367d939):
With
"build": "mkdir out"in both packages, the second run oflegacyfails withmkdir: cannot create directory 'out': File existsandbun run --filter '*' buildexits 1. With the fix it exits 0.Other
"workspaces"values that gave a duplicate before the fix, all checked by hand:["packages/*", "packages/l*"],["packages/*", "packages/**"],["packages/*", "packages/{legacy,app}"]["packages/legacy", "packages/legacy"]["packages/*", "./packages/legacy"],["packages/*", "packages/legacy/"],["packages/*", "packages//legacy"],["packages/*", "packages/../packages/legacy"]["packages/*", "!!packages/legacy"]{"packages": ["packages/*", "packages/legacy"]}With
BUN_DEBUG_Glob=1on a debug build, each of the spellings above logs the sameprepared match: <root>/packages/legacy/package.jsonfrom both walks. The walks run in absolute mode, andGlobWalker::joinnormalizes the path in that mode. This is why the yielded path works as the key with no more normalization.The key is the byte-exact path, like
bun install. Two paths that differ only in letter case on a case-insensitive file system are two keys in both places.Why the set sits in
select_packagesand not inPackageFilterIterator::next: the set then covers each path the iterator returns, whatever produces it. Other open work on this code:PackageFilterIteratorand writes the walk as afor pattern in &glob_patternsloop inselect_packages. It has no check for a repeated path. If it lands after this PR, the check must move into that loop.robobun/d3bfc08a/run-filter-negated-workspacesapplies!entries inPackageFilterIterator::next. It does not touch the loop inselect_packages.Not in this PR:
bun run --filterruns a package twice when a symlink that a "workspaces" glob matches points to it #43358. Withpackages/alias -> realand"workspaces": ["packages/*"], the one walk returnspackages/alias/package.jsonandpackages/real/package.json, andrealruns twice.bun installrecords one member. The existing testself-referential directory symlink in a workspace does not loopasserts this result (scriptcyc: 2), so this PR leaves it as it is.bun runandbun installfind the members with separate code (select_packagesandWorkspaceMap::process_names_array). One shared step would remove this class of difference.bun run --filterruns a package twice when a symlink that a "workspaces" glob matches points to it #43358 notes it. The tests here only count script runs through the CLI, so they stay valid for that change."<root>/packages/legacy". It makesbun run --filterand--workspacesfail witherror: Unexpectedbefore any script runs, with and without this PR. It is tracked inbun run --filterfails witherror: Unexpectedwhen a "workspaces" entry is an absolute path #43360. An absolute entry with a glob ("<root>/packages/*") works, and this PR removes its repeats too.bun testhas the same kind of repeat for path arguments:bun test --isolate ./sub ./sub/b.test.tsruns the file twice. It is in other code (src/runtime/cli/test/Scanner.rs), tracked inbun testruns a file twice when two path arguments select it (bun test --isolate ./sub ./sub/b.test.ts) #43359.The self-review raised three points. State how the bug was found and why it matters (the exit 1). List the open PRs that touch this loop, and track what this PR leaves out (#43358, #43359). Cover the
{"packages": [...]}form in the tests. This version has all three.Suites run with the debug build:
test/cli/run/filter-workspace.test.ts(95 pass, 2 skip on Linux),test/cli/run/multi-run.test.ts(127 pass),test/cli/run/workspaces.test.ts,test/cli/run/run-quote.test.ts,test/regression/issue/26207.test.ts,test/regression/issue/31636.test.ts.