Conversation
|
Warning Review limit reached
Next review available in: 9 minutes 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 ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughInstall initialization now loads options earlier and handles loading errors through the out-of-memory path. A CLI test and documentation updates cover ChangesInstall cache option loading
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 4:02 PM PT - Jul 28th, 2026
❌ @robobun, your commit 35045a7 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 36219That installs a local version of the PR into your bun-36219 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/PackageManager.rs (1)
2032-2042: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove
BUN_MANIFEST_CACHEafteroptions.load()
Options::load()can clearmanifest_cacheandmanifest_cache_controlvia config-driven flags likedisable_manifest_cacheandforce, so applying the env override before it lets those settings win. Set the env override afterload()in both init paths, then keep the later!cache()reset so an explicit global cache disable still wins.🤖 Prompt for 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. In `@src/install/PackageManager.rs` around lines 2032 - 2042, Move the BUN_MANIFEST_CACHE environment override logic from before options.load() to after options.load() in both initialization paths, preserving its manifest_cache and manifest_cache_control assignments. Keep the later !cache() reset unchanged so an explicit global cache disable still takes precedence.
🤖 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.
Outside diff comments:
In `@src/install/PackageManager.rs`:
- Around line 2032-2042: Move the BUN_MANIFEST_CACHE environment override logic
from before options.load() to after options.load() in both initialization paths,
preserving its manifest_cache and manifest_cache_control assignments. Keep the
later !cache() reset unchanged so an explicit global cache disable still takes
precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 873a5eae-47b9-47ec-91a5-b26c02ccfb98
⛔ Files ignored due to path filters (1)
test/cli/install/__snapshots__/bun-security-scanner-matrix-with-node-modules.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/install/PackageManager.rs
|
Re the suggestion to move |
There was a problem hiding this comment.
LGTM — d1feb28 addresses the precedence concern from my earlier review; only the if !enable.cache() propagation now moves past options.load().
What was reviewed:
- Verified the final ordering in both
init()andinit_with_runtime_once():BUN_MANIFEST_CACHE→load()→ cache-propagation.Subcommand::Update/--no-cache/--forceclears insideload()still win, and the propagation only ever clears flags so it can't re-enable anything. - Confirmed against
PackageManagerOptions.rsthatconfig.disable_cacheclears onlyEnable::CACHE(line 468) while the boolean-shorthand path also setsdisable_manifest_cache— so the moved propagation is exactly what makes the two spellings equivalent. - Snapshot regen is confined to the
bun.lock exists: falserequested-packagesentries; the new values are internally consistent across the linker/scanner axes and match the mechanism (previous[]was the pre-install's leaked manifest cache hiding the real requests).
Extended reasoning...
Overview
Moves the 3-line if !enable.cache() { clear manifest_cache + manifest_cache_control } block from before options.load() to after it in both PackageManager init paths. The block was previously dead (Enable::CACHE defaults on, nothing cleared it before load()), so bunfig's [install.cache] disable = true never propagated to the manifest cache. Also adds a targeted test in bun-install.test.ts and regenerates the bun-security-scanner-matrix-with-node-modules snapshots that had captured the buggy/racy behaviour.
Security risks
None. This narrows caching (more registry requests, never fewer) and touches no auth/crypto/permission paths.
Level of scrutiny
Low-to-moderate. The Rust change is a pure 5-line block relocation ×2 with no new logic; the propagation only clears flags so it can't undo the disables that load() applies for bun update / --no-cache / --force. The large snapshot diff is mechanical and matches the described mechanism — every changed entry is a (bun.lock exists: false) requested-packages case going from an empty/partial list to the full resolution set, which is exactly what happens when the pre-install's manifest cache stops leaking into the measured install.
Other factors
My earlier review flagged that the first revision inverted BUN_MANIFEST_CACHE vs load() precedence; d1feb28 fixed this by keeping the env-var block before load() and moving only the cache-propagation past it, which I've verified against the current file contents. The new test uses the file's established withContext/dummyRegistryForContext harness, asserts no *.npm in node_modules/.cache, and asserts a second install re-fetches the manifest — both assertions target the specific bug. No CODEOWNERS cover the touched paths.
There was a problem hiding this comment.
Thanks for the follow-up in d1feb28 — the BUN_MANIFEST_CACHE block is back before options.load() in both init paths, so Subcommand::Update / --no-cache / --force precedence is preserved and only the previously-dead !enable.cache() propagation moved. No further issues found.
Deferring to a human for final sign-off since this is an intentional user-facing behavior change ([install.cache] disable now subsumes disableManifest, vs. the previously-documented "may still write to node_modules/.cache") and comes with a ~1000-line scanner-matrix snapshot regen that's worth a spot-check.
Extended reasoning...
Overview
The PR moves a 5-line if !enable.cache() { disable manifest cache } block from before options.load() to after it, in both PackageManager::init() and init_with_runtime_once(). This makes [install.cache] disable = true in bunfig actually propagate to the manifest cache — previously the guard ran against Enable::default() (which has CACHE set) so it was dead code. Docs for install.cache.disable are updated in two places, a new test is added to bun-install.test.ts, and the bun-security-scanner-matrix-with-node-modules snapshot is regenerated.
Prior review
I previously flagged that an earlier revision moved the BUN_MANIFEST_CACHE env-var block after options.load(), which would have let BUN_MANIFEST_CACHE=2 bun update re-enable the manifest cache after load() cleared it. That was fixed in d1feb28: the env-var block stayed put and only the !enable.cache() propagation moved. I verified the current diff — both init paths now have the order BUN_MANIFEST_CACHE → options.load() → !enable.cache() propagation, which preserves the Subcommand::Update / --no-cache / --force invariants while fixing the bunfig case. That thread is resolved.
Security risks
None. This only tightens caching (disable propagates further); worst case is more registry requests, not fewer security checks.
Level of scrutiny
Medium. The Rust change is 10 lines net-zero and mechanically obvious once the ordering is right. However, this is package-manager init and a deliberate change to documented bunfig semantics: disable and disableManifest were previously independent knobs per the old docs, and disable = true now subsumes disableManifest. The PR description makes the case (consistency with the install.cache = false shorthand, fixes #21635, de-flakes #36215), but a maintainer should confirm they agree with the semantic change.
Other factors
The snapshot regen (~1000 lines across ~200 entries) is well-explained in the PR description — bun.lock exists: false cases go from [] to the full resolution set, and install|add left-pad,is-even cases with bun.lock exists: true gain left-pad because explicitly-named args now hit the registry. This is consistent with disabling the manifest cache, but worth a human spot-check given the volume.
|
CI on 35045a7 (rebased) is green for everything this PR touches: the new Remaining lane failures are unrelated to the diff:
Ready for review. This is a deliberate user-facing semantic change ( |
Options::load() is what applies bunfig's disable_cache to Enable::CACHE, but the 'cache disabled => manifest cache disabled' propagation and the BUN_MANIFEST_CACHE override both ran before load(), against a default-initialised Enable that always has CACHE set. The block has been dead since 3915e01 moved the eager cache-dir open (which used to clear Enable::CACHE on failure) into the lazy ensure_cache_directory. Moving the block after load() in both init paths makes '[install.cache] disable = true' behave like 'install.cache = false': no .npm manifest files are written and a second install hits the registry for the manifest again.
…x snapshots Move only the cache->manifest propagation past options.load(), not the BUN_MANIFEST_CACHE override. Keeping the env var before load() means Subcommand::Update / --no-cache / --force still win as before. The security-scanner matrix (with-node-modules) used cache.disable = true and its snapshots captured [] for requested-packages on runs where the pre-install's manifest-cache write finished before exit. With the manifest cache actually disabled those runs now make the manifest requests deterministically; the snapshots for bun.lock-exists=false cases are regenerated to match.
The previous wording ("don't load from the global cache. Bun may still
write to node_modules/.cache") described the old behavior where
disable only switched the cache directory. disable = true now matches
install.cache = false, and disableManifest remains as the
manifest-only knob.
03bf839 to
35045a7
Compare
There was a problem hiding this comment.
Confirmed the precedence fix — in both init() and init_with_runtime_once(), BUN_MANIFEST_CACHE now stays before options.load() and only the !enable.cache() propagation moved after it, so Subcommand::Update / --no-cache / --force still win. No further findings.
What was reviewed:
- Reordering in
PackageManager.rsagainst theUpdate/--no-cache/--forceclears inPackageManagerOptions.rs::load()— precedence unchanged from main. - New
bun-install.test.tscase: uses local dummy registry, asserts no*.npminnode_modules/.cache, and asserts the manifest is re-fetched on the second install. - Regenerated scanner-matrix snapshots: consistent with the manifest cache now being disabled under
[install.cache] disable = true.
Deferring for a maintainer to sign off on the deliberate semantic change (disable subsuming disableManifest), as the author noted.
Extended reasoning...
Overview
The PR moves a previously-dead if !enable.cache() { disable manifest cache } block from before options.load() to after it in both PackageManager::init() and init_with_runtime_once(). This makes [install.cache] disable = true in bunfig also disable the npm manifest cache, matching the existing install.cache = false shorthand. Docs are updated to reflect the new semantics, a new test in bun-install.test.ts covers it, and the bun-security-scanner-matrix-with-node-modules snapshots were regenerated (large but mechanical — the previous [] values were manifest-cache-served results, now they show the actual registry requests).
Prior feedback
My earlier review flagged that the first revision moved the BUN_MANIFEST_CACHE env-var block after load(), which would have let BUN_MANIFEST_CACHE=2 bun update re-enable the manifest cache after load() cleared it. The author addressed this in d1feb28: only the !enable.cache() propagation moved; the env-var block is back to its original position before load(). I verified this in the current diff at both call sites (PackageManager.rs:2032-2058 and 2417-2445). The inline thread is resolved.
Security risks
None. This only affects when Bun consults its own on-disk manifest cache vs. hitting the configured registry. No auth, crypto, or untrusted-input parsing is touched.
Level of scrutiny
Medium. The Rust change is a 10-line reorder of existing logic with a well-argued cause (the block was checking a default that hadn't been loaded yet). The snapshot churn is large but mechanical and CI-verified. What warrants human attention is the deliberate user-facing behavior change: previously disable and disableManifest were documented as independent ("Bun may still write to node_modules/.cache"); now disable = true subsumes disableManifest. That's an API-design call the author explicitly flagged for maintainer sign-off, and it fixes #21635 where a user reported the old behavior as a bug — so it's likely the right call, but not one I should make unilaterally.
Other factors
CI on 03bf839 is green for the touched tests per the author's report; remaining lane failures are unrelated flakes. The new test uses the local dummy registry (no external network), asserts on observable state (*.npm files, request URLs), and would fail on the unfixed build per the PR description.
This branch has not been deployed
1 participant
What
[install.cache] disable = truein bunfig left the npm manifest cache enabled, so*.npmmanifest files were still written tonode_modules/.cacheand a subsequent install could read them instead of asking the registry. The shorthandinstall.cache = falsealready disabled both. Surfaced as nondeterministic registry-request counts in the security-scanner matrix tests (#36215), where the async manifest-save raced process exit.This is a behavior change:
disableanddisableManifestwere previously independent knobs (docs describeddisableas "don't load from the global cache. Bun may still write to node_modules/.cache"). After this PR,disable = truematchesinstall.cache = falseand subsumesdisableManifest;disableManifest = truealone remains useful for "keep using the global cache directory, always fetch fresh manifests". Docs updated.Repro
Install once, then again after removing the lockfile + installed package but keeping
node_modules/.cache:.npminnode_modules/.cache[install.cache] disable = true(before)install.cache = false[install.cache] disable = true(after)Cause
Both
PackageManagerinit paths (initfor the CLI,init_with_runtime_oncefor auto-install) hadif !manager.options.enable.cache() { disable manifest cache }beforeoptions.load().Enable::default()hasCACHEset, so the guard was always false. The block dates to a765b13 where it ran after an eager cache-dir open that clearedEnable::CACHEon failure (and which already cleared the manifest flags inline, so the block was redundant even then); 3915e01 made the open lazy and the block went fully dead.Fix
Move the
if !enable.cache()propagation to afteroptions.load()in both init paths so it sees the bunfig-loaded value. TheBUN_MANIFEST_CACHEblock stays beforeload()soSubcommand::Update,--no-cache, and--force(which clear the manifest-cache flags insideload()) still win as before.Verification
New test in
test/cli/install/bun-install.test.ts: install with[install.cache] disable = true, assert no*.npmfiles land innode_modules/.cache, and assert a second install (lockfile removed,.cachekept) still fetches the manifest from the registry.bun-security-scanner-matrix-with-node-modules.test.tsusedcache.disable = trueand itsrequested-packagessnapshots had captured manifest-cache-served results. Regenerated: thebun.lock exists: falsecases go from[]to the full resolution set, and 60bun.lock exists: truecases forinstall|add left-pad,is-evengain"left-pad"(explicitly-named, already-locked args now hit the registry instead of the pre-install's cached manifest). Thewithout-node-modulesvariant is unaffected since it removesnode_modules/.cachebetween the pre-install and the measured run.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/install/bun-install.test.ts