Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 5 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (9)
Comment |
|
Updated 6:59 PM PT - Aug 21st, 2026
❌ @robobun, your commit 26eca05 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38923That installs a local version of the PR into your bun-38923 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The implementation reuses the disk-check pattern from bun pm licenses and is well-tested. Because it changes user-facing bun pm ls semantics (filtering to disk contents, and now erroring when node_modules is missing) and updates an existing test's documented expectation, a human look would still be worthwhile to confirm the design direction.
What was reviewed:
InstalledFilterpath construction viaAutoAbsPath::append— separator handling matches the existingBunStore/tree_locationsusage inpm_licenses_command.rs.- The isolated-linker fallback via
BunStore::lookup— same lazy scan already exercised bybun pm licensesand covered by thefrozen-lockfile-prunedisolated case. - The non-
--allpath passesb"node_modules"(no trailing sep) tois_installed;appendinserts the separator, so root deps resolve to<top>/node_modules/<alias>correctly. Root/Workspace/Symlinkresolutions bypass the disk check, so themoo@moofolder-dep and workspace tests keep listing local sources.
Extended reasoning...
Overview
The PR changes bun pm ls to filter lockfile entries by disk existence before listing/counting them, fixing #38909 where platform-specific optionals and pruned packages inflated the count. It adds an InstalledFilter struct in package_manager_command.rs that checks each dependency's hoisted path via bun_sys::exists, falling back to the isolated-install store via a now-pub(crate) BunStore::lookup from pm_licenses_command.rs. Help text and shell completions are updated to say "installed dependencies" instead of "according to the current lockfile". Two new tests are added and one existing snapshot in frozen-lockfile-pruned.test.ts is updated with a stated rationale.
Security risks
None. The change reads directory existence under the project's own node_modules; no new user input is parsed, no network, no privilege changes.
Level of scrutiny
Medium. The code itself is straightforward and closely mirrors the already-shipped bun pm licenses disk-check logic (same AutoAbsPath::init_top_level_dir + append + bun_sys::exists shape, same BunStore scan). The Rust changes are ~80 lines of new code plus visibility widening. However, this is a user-visible behavior change to a CLI command, and it introduces a new failure mode: bun pm ls now exits 1 with "node_modules not found" when the directory is absent, where it previously listed lockfile contents. That matches bun pm licenses but is still a design decision.
Other factors
- The updated
frozen-lockfile-pruned.test.tscase previously carried a comment citingdocs/pm/cli/install.mdx("lockfile-driven commands still see them"). The PR replaces that comment and changes the assertion; the PR description explains why, but a maintainer should confirm this is the intended semantics forpm ls(vs., say, adding a--lockfileflag) and whether any docs need a matching edit. - The
Root/Workspace/Symlinkearly-return means workspaces are always listed even if their symlink undernode_modulesis missing; this is deliberate (the frozen-lockfile test relies on it) but is another small design call. - Test coverage is solid: both new tests demonstrably fail on the released bun per the PR description, the isolated-linker path is exercised by the existing
describe.eachmatrix, and the full suite runs listed in the description are comprehensive. - No prior reviews from me or other reviewers on this PR.
|
Thanks for the look. On the two design points: erroring when node_modules is missing and always listing workspace/root/link resolutions both mirror what bun pm licenses already ships, so pm ls now follows the same semantics rather than inventing new ones. The docs page for pm ls already described the command as listing installed dependencies, so no docs edit is needed beyond the help text updated here. |
ad0832c to
b040d64
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes user-facing CLI semantics — bun pm ls now hard-errors when node_modules is missing where it previously succeeded, and an existing test's contract was inverted — a human sign-off on those design calls would still be worthwhile. There are also three open comment-cop flags on package_manager_command.rs.
Checked that InstalledFilter reuses the same AutoAbsPath::init_top_level_dir() + BunStore path logic that bun pm licenses already ships, so the hoisted-path check and isolated-store fallback match an existing pattern.
Checked that the non---all branch's sorted_dependencies retain uses the fixed prefix b"node_modules" (no trailing separator) — AutoAbsPath::append inserts the separator, matching the tree iterator's relative_path shape.
Verified the frozen-lockfile-pruned.test.ts snapshot change drops only left-pad (the never-installed exclusive dep) while keeping the workspace entry, consistent with the new filter treating ResolutionTag::Workspace as always-present.
Extended reasoning...
Overview
The PR changes bun pm ls from a lockfile-tree printer to a disk-filtered listing. Core change is a new InstalledFilter struct in src/runtime/cli/package_manager_command.rs (~75 lines) that checks each lockfile entry against node_modules/<tree-path>/<alias> and, as a fallback, the isolated-install store via BunStore::lookup (newly pub(crate) from pm_licenses_command.rs). The filter is applied to both the --all tree walk and the top-level listing, and installed_count replaces hoisted_dependencies.len() in the header. A new early-exit errors when node_modules doesn't exist. Help text and shell completions are reworded. Two new tests in bun-pm.test.ts; one existing snapshot in frozen-lockfile-pruned.test.ts inverted to match the new semantics.
Security risks
None identified. The change reads local filesystem paths derived from lockfile entries the user already installed; no untrusted input parsing, no network, no auth. bun_sys::exists is used for the disk probe, matching the in-tree syscall wrapper convention.
Level of scrutiny
Medium. This is a CLI display command, not a hot runtime path or install logic — but it introduces a new hard-error exit path (node_modules missing → exit 1) where the command previously succeeded, which is a user-facing behavior change that could break scripts. The PR description justifies it by parity with bun pm licenses, and the author has already responded to that design point in the thread, but a maintainer should confirm they want that parity.
Other factors
- Three unaddressed comment-cop inline flags on the new comments in
package_manager_command.rs(lines 44, 83, 105) are still open on the PR. - One existing test (
frozen-lockfile-pruned.test.ts) had its documented contract inverted — the PR description explains why, and the reasoning is sound (the dropped package was genuinely never installed), but per the repo review rules this is exactly the kind of test-semantics change that should get a human ack. - Test coverage for the new behavior looks solid: both
lsandls --allafter deletion, the new error path, and the isolated-linker case is covered indirectly via the existingfrozen-lockfile-prunedmatrix.
|
The three comment-cop threads are replied to and resolved (the flagged comments state intended semantics rather than justify workarounds). The two design points, erroring when node_modules is missing and the frozen-lockfile-pruned snapshot change, are covered in the PR description and the earlier comment: both follow the semantics bun pm licenses already ships. Leaving the final call on that direction to a maintainer. |
### Problem - `bun pm ls` without `--all` prints a root dependency once per entry the root has for it. A workspace `a` that is also in the root's `devDependencies` prints twice under a `node_modules (1)` header. So does a name declared in two groups (`typescript` in #8283's example). - Cause: `src/runtime/cli/package_manager_command.rs:644` prints the root's dependency list as is: one entry per declaration plus one per workspace member. `node_modules` has one folder per name. ### Fix - Stable sort the root's entries by name and drop the later entries of a name (`dedup_by`). `--trusted` filters the result. - Correct because one name is one `node_modules/<name>` folder, the same collapse `Tree::hoist_dependency` applies. The list is in the tree builder's order, so the entry kept is the one the tree kept. An alias has its own name and keeps its row. The listing does not read the stored tree, so an older `bun.lockb` prints as before (Notes). - Verified: `test/cli/install/bun-pm.test.ts`. Three new tests (bun.lock, bun.lockb, `--trusted`) fail on the released bun. One more pins that a root optional peer bound to a dependency's copy stays listed. On this repo's lockfile the only change is the removed `@types/bun` row. ### Background - Each package owns one range of `buffers.dependencies`. The root's range holds its declared dependencies plus one `Behavior::WORKSPACE` entry per workspace member, sorted by group and name. Entries of one name share one folder. - The hoisted tree (`buffers.trees`, `buffers.hoisted_dependencies`) models `node_modules`: one tree per folder, one entry per name. `--all` prints it and the header `(N)` counts its entries. The default listing prints only the root's range. <details><summary>Notes</summary> Repro without a registry: ```sh mkdir -p d/packages/a && cd d echo '{"name":"root","workspaces":["packages/*"],"devDependencies":{"a":"workspace:*"}}' > package.json echo '{"name":"a","version":"1.0.0"}' > packages/a/package.json bun install && bun pm ls ``` Released bun prints `a@workspace:packages/a` on two lines, three when `a` is also in `dependencies`. `bun pm ls --all` prints it once. `bun pm ls --trusted` with `trustedDependencies: ["a"]` prints it twice. In this repo `bun pm ls` prints `@types/bun@workspace:packages/@types/bun` twice. - The first version of this PR printed the tree's root folder filtered to the root's id range. A self-review found a case it drops: a root optional peer that a dependency provides. When the tree builder binds that peer late, it stores the folder entry under the provider's dependency id (`Tree.rs`, `ResolveReplace`). bun 1.4.0 re-hoists on load and on a second pass stores the root's own id, but a `bun.lockb` written by bun 1.3.x keeps the provider's id, and bun does not rewrite a lockfile that needs no changes. Checked with a `bun.lockb` written by bun 1.3.14 from the `optional-peer-hoist-provider` and `-target` registry fixtures: the tree version listed only the provider, the current version lists both, as 1.3.14 and the released 1.4.0 do. The new optional peer test covers the fresh lockfile shape of this case. - Dedupe by package id was rejected: `bar` and `bar-alias: npm:bar` share one package id but are two folders. The test keeps both rows. - The `--trusted` test uses bun.lock only. bun.lockb stores only hashes of `trustedDependencies`, and `has_trusted_dependency` does not trust a hash alone since #31339, so `--trusted` lists nothing from a bun.lockb. Released bun does the same. Not related to this change. - A real conflict (workspace `a` plus `"a": "file:..."` at the root) does not reach `pm ls` today: `bun install` writes a duplicate package path and the lockfile fails to load (the area of #33156). - #8283 asks for grouped output and `--json`. This PR removes the duplicate row in its example and nothing else, so it does not close that issue. - Open PRs that touch this branch and do not fix this: #38923 (filter by what is on disk), #38952 (header wording). The fold branch of #39403 has the same loop. - Suites run with the debug build: `test/cli/install/bun-pm.test.ts` (22 pass), `bun-install-lifecycle-scripts.test.ts -t trusted` (55 pass), `bun-install-registry.test.ts -t "migration is out of sync"`, `test/regression/issue/24502`. `cargo fmt --check` is clean. `bun pm ls --all` on this repo is unchanged byte for byte. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 3 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/install/bun-pm.test.ts bun test v1.4.0 (6e906e4) test/cli/install/bun-pm.test.ts: (pass) should list top-level dependency [386.82ms] (pass) should list all dependencies [333.90ms] (pass) should list top-level aliased dependency [329.02ms] (pass) should list aliased dependencies [346.24ms] (pass) should list only trusted dependencies with --trusted [464.07ms] (pass) should list only trusted dependencies with --all --trusted [339.68ms] (pass) should list trusted transitive dependencies under untrusted parents with --all --trusted (isolated) [356.01ms] (pass) should list nothing with --trusted when no dependencies are trusted [346.58ms] 597 | expect(await exists(join(package_dir, lockfile))).toBeTrue(); 598 | urls.length = 0; 599 | 600 | const [stdout, stderr, exitCode] = await spawnAndCollect("pm", "ls"); 601 | expect(stderr).toBe(""); 602 | expect(normalizeBunSnapshot(stdout, package_dir)).toMatchInlineSnapshot(` ^ error: expect(received).toMatchInlineSnaps ... (truncated) release without fix: 3 FAILED bun test v1.4.0-canary.1 (6e906e4) test/cli/install/bun-pm.test.ts: (pass) should list top-level dependency [17.59ms] (pass) should list all dependencies [15.04ms] (pass) should list top-level aliased dependency [18.13ms] (pass) should list aliased dependencies [26.89ms] (pass) should list only trusted dependencies with --trusted [29.53ms] (pass) should list only trusted dependencies with --all --trusted [30.36ms] (pass) should list trusted transitive dependencies under untrusted parents with --all --trusted (isolated) [19.13ms] (pass) should list nothing with --trusted when no dependencies are trusted [20.57ms] 597 | expect(await exists(join(package_dir, lockfile))).toBeTrue(); 598 | urls.length = 0; 599 | 600 | const [stdout, stderr, exitCode] = await spawnAndCollect("pm", "ls"); 601 | expect(stderr).toBe(""); 602 | expect(normalizeBunSnapshot(stdout, package_dir)).toMatchInlineSnapshot(` ^ error: expect(received).toMatchInlineSnapshot(expected) "<dir> node_modules (5) ├── bar@0.0.2 + ├── bar@0.0.2 ├── bar-alias@0.0.2 ├── ws-once@workspace:packages/ws-once + ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/install/bun-pm.test.ts bun test v1.4.0 (6e906e4) test/cli/install/bun-pm.test.ts: (pass) should list top-level dependency [381.86ms] (pass) should list all dependencies [333.18ms] (pass) should list top-level aliased dependency [351.49ms] (pass) should list aliased dependencies [347.67ms] (pass) should list only trusted dependencies with --trusted [463.21ms] (pass) should list only trusted dependencies with --all --trusted [348.99ms] (pass) should list trusted transitive dependencies under untrusted parents with --all --trusted (isolated) [321.16ms] (pass) should list nothing with --trusted when no dependencies are trusted [309.86ms] (pass) should list a workspace the root also depends on once (bun.lock) [541.88ms] (pass) should list a workspace the root also depends on once (bun.lockb) [314.14ms] (pass) should list a trusted workspace the root also depends on once with --trusted [315.39ms] (pass) should list a root optional peer that a dependency provides [328.74ms] (pass) should remove all cache [465.82ms] (pass) bun pm m ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 683ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/5] gen generated_host_exports.rs generated_host_exports.rs: 92 exports (host=3, lazy=10, generic=79, rust=0); 240 extern-C blocks audited [1/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_runtime v0.0.0 (/workspace/bun/src/runtime) �[1m�[92m Compiling�[0m bun_bin v0.0.0 (/workspace/bun/src/bun_bin) �[1m�[92m Finished�[0m `release` profile [optimized + debuginfo] target(s) in 4m 36s [2/5] link bun-profile [4/5] strip bun [4/5] bun-profile --revision 1.4.0-canary.1+b263001b0 [build] done bun test v1.4.0-canary.1 (b263001) test/cli/install/bun-pm.test.ts: (pass) should list top-level dependency [15.40ms] (pass) should list all dependencies [11.19ms] (pass) should list top-level aliased dependency [9.40ms] (pass) should list aliased dependencies [9.71ms] (pass) should list only trusted dependencies with --trusted [10.18ms] (pass) should list on ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/cli/package_manager_command.rs | 8 +- test/cli/install/bun-pm.test.ts | 150 ++++++++++++++++++++++++++++- 2 files changed, 152 insertions(+), 6 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/cli/package_manager_command.rs 5 4 0 test/cli/install/bun-pm.test.ts 10 9 0 ``` </details> **root cause** · written by the author bot The root package's dependency list receives one entry per workspace member plus one per declared dependency, so a workspace that the root also declares as a dependency has two entries resolving to the same package, and the default bun pm ls loop printed each of them. The fix sorts the root's entries by name with a stable sort and then collapses adjacent entries with the same name, so each package is printed once, while the --trusted filtering in the same branch is unaffected. <!-- robobun:evidence:end -->
bun pm ls printed the lockfile's hoisted dependency list, so the header counted platform-mismatched optional dependencies that were never installed, and packages removed from disk (bun prune --production) were still listed. Filter entries against the disk like bun pm licenses does, checking the hoisted path first and the isolated store as a fallback, and error when node_modules does not exist at all.
b040d64 to
26eca05
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes user-facing bun pm ls semantics (lockfile → disk), adds a new hard-error path when node_modules is missing, and rewrites an existing test's documented expectation, a maintainer sign-off on the direction is still warranted — the author noted the same.
Checked: InstalledFilter::is_installed path construction against AutoAbsPath::append semantics (matches pm_licenses' segment-append usage, handles nested node_modules/…/node_modules relative paths); the BunStore fallback covers isolated-linker transitive packages so they aren't wrongly dropped; the top-level (non---all) branch's separate retain filter uses the correct b"node_modules" relative path for root deps; init_top_level_dir() is safe because PackageManager::init already ran.
Extended reasoning...
Overview
The PR redefines bun pm ls to report packages actually present on disk rather than every lockfile entry. Core logic is a new InstalledFilter in package_manager_command.rs that checks each dependency's hoisted path (and, as a fallback, the isolated-install store via BunStore, promoted to pub(crate) from pm_licenses_command.rs). Root/workspace/symlink resolutions are always kept. When node_modules is absent, the command now exits 1 with an error and a run 'bun install' first note. Help text and shell completions are reworded, two new tests are added to bun-pm.test.ts, and one snapshot in frozen-lockfile-pruned.test.ts is updated to drop a never-installed dependency.
Security risks
None. The change reads local filesystem state (bun_sys::exists) and adds no new inputs, network access, or path traversal surface beyond what pm licenses already exercises with the same helpers.
Level of scrutiny
Medium. The implementation is straightforward and mirrors already-shipped bun pm licenses semantics, and the bug-hunting pass found nothing. But it is a deliberate user-facing behavior change: the header count changes, packages disappear from output, and bun pm ls newly hard-fails before install. Scripts or CI that run pm ls on a fresh checkout will now exit non-zero. The frozen-lockfile-pruned test previously documented the opposite behavior ("lockfile-driven commands still see them") and its comment/assertion were rewritten to match the new semantics.
Other factors
The author's own final comment on the thread explicitly leaves the design call (error-on-missing-node_modules and the pruned-snapshot change) to a maintainer. Given that, plus the modified existing-test expectation and the new failure mode, this warrants a human look before merging even though I found no correctness issues in the code itself.
|
CI note: the remaining red in build 103022 is test/cli/install/bun-audit.test.ts, which is broken on main (the 1.4.1 version bump makes normalizeBunSnapshot rewrite the advisory range |
This branch has not been deployed
1 participant
Fixes #38909
Problem
bun pm lsprints a header labellednode_modules (N)where N islockfile.buffers.hoisted_dependencies.len(), and the listing walks the lockfile's dependency tree. Nothing ever checks the disk.--alllists them.bun prune --production, manual deletion) stay listed.bun pm licensesalready checks the disk and gets both cases right.Fix
src/runtime/cli/package_manager_command.rs: filter every entry through a disk-existence check before listing or counting. A dependency counts as installed when its directory exists at its hoisted path (<tree position>/<alias>), or, as a fallback, when it has an entry in the isolated-install store (node_modules/.bun/<name@version>), since isolated installs only link direct dependencies into each package'snode_modules.bun linkresolutions are always kept: they are local sources that live outsidenode_modules.BunStorefrombun pm licenses(nowpub(crate)), which also handles peer-suffixed store keys.node_modulesdoes not exist at all,bun pm lsnow errors withnode_modules not found, nothing to listand arun 'bun install' firstnote, matchingbun pm licenses.bun pm lsalready said "installed dependencies".test/cli/install/bun-pm.test.ts(deleted package disappears from listing and count; missingnode_moduleserrors), both fail on the released bun and pass with this change. Full runs ofbun-pm.test.ts(20/20),frozen-lockfile-pruned.test.ts(101/101, hoisted and isolated linkers),bun-pm-licenses.test.ts(79/79), plus thepm lstests inbun.test.ts,bun-install-registry.test.ts, andbun-install-lifecycle-scripts.test.ts.One existing test changed behavior:
frozen-lockfile-pruned.test.tsasserted that after a pruned-monorepo install skips a workspace,bun pm ls --allstill lists the skipped workspace's exclusive dependency. That dependency was never installed, which is exactly the case this issue reports, so the snapshot now omits it. The skipped workspace itself is still listed.Background
bun.locklegitimately contains packages that will never exist on a given machine (for example@esbuild/win32-x64on Linux). The lockfile is not a description ofnode_modules; the install step decides what actually lands on disk.node_modules/a/node_modules/b). Isolated installs (nodeLinker: isolated) instead keep one copy per package undernode_modules/.bun/<name@version>/and symlink only direct dependencies into each package'snode_modules, which is why the hoisted path check alone would wrongly drop transitive packages in isolated mode.no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/frozen-lockfile-pruned.test.ts, test/cli/bun.test.ts