Conversation
…n set The env var was read with the comparison inverted since it was added: BUN_CONFIG_NO_VERIFY=0 turned tarball integrity verification off and BUN_CONFIG_NO_VERIFY=1 left it on. Read it the same way as the BUN_CONFIG_SKIP_* variables next to it, so any value other than "0" skips verification (same as --no-verify) and "0" keeps it enabled.
|
Warning Review limit reached
Next review available in: 43 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 selected for processing (2)
Comment |
|
Reproduced on main (b7a0431) with a loopback registry that advertises the sha512 of a different tarball than the one it serves: With this branch the two env var rows flip ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-character polarity fix aligning BUN_CONFIG_NO_VERIFY with the three BUN_CONFIG_SKIP_* env vars directly above it.
What was reviewed:
- Confirmed
BUN_CONFIG_NO_VERIFYappears nowhere else in the repo (src, tests, harness, docs), so the polarity flip has no collateral impact. - The three new tests reuse the existing
serveManifest/projectDirhelpers and drain/assert pattern from the same describe block; the=1and=0cases each fail on main for the reason stated. --no-verifyis applied after the env var inOptions::load, so CLI-flag precedence is unchanged; disabling verification via env adds no capability beyond the already-documented--no-verify.
Extended reasoning...
Overview
Single behavioral change at src/install/PackageManager/PackageManagerOptions.rs:672: check_bool != b"0" → check_bool == b"0". This makes BUN_CONFIG_NO_VERIFY=<truthy> clear Do::VERIFY_INTEGRITY (skip integrity checks) and BUN_CONFIG_NO_VERIFY=0 keep it set — the same reading used by BUN_CONFIG_SKIP_SAVE_LOCKFILE, BUN_CONFIG_SKIP_LOAD_LOCKFILE, and BUN_CONFIG_SKIP_INSTALL_PACKAGES in the immediately preceding lines. Three tests are added to test/cli/install/bun-install-tarball-integrity.test.ts inside the existing tarball integrity metadata forms describe block, covering --no-verify, BUN_CONFIG_NO_VERIFY=1, and BUN_CONFIG_NO_VERIFY=0 against a registry that advertises a mismatched sha512.
Security risks
None introduced. The change makes it possible to disable tarball integrity verification via an env var, but that capability already exists via the documented --no-verify CLI flag (handled a few dozen lines later in the same function), and the env var itself already existed — it just had inverted polarity. Grep confirms the env var is undocumented and unused elsewhere in the repo, so no CI/test was silently relying on the old inverted behavior to keep verification on.
Level of scrutiny
Low. This is a one-token bug fix that brings one env var in line with three identically-shaped siblings right above it. The fix is mechanically obvious once the bug is stated, and the PR description traces the inversion back to the 2022 commit that introduced it. The tests are the bulk of the diff and follow the file's existing conventions exactly (same tarball builder, same local Bun.serve registry, same Promise.all pipe drain, describe.concurrent, per-test cache dir).
Other factors
- Repo-wide grep confirms this is the only read of
BUN_CONFIG_NO_VERIFY;bunEnv/harness does not set it, so flipping polarity cannot break other tests. - CLI
--no-verifyis applied after the env-var block inOptions::load, so flag-overrides-env precedence is preserved. - The new tests satisfy the review guide's "cover the variant matrix" rule (env=1, env=0, and the CLI flag they mirror) and each has a clear fails-on-main / passes-here direction per the PR description.
- No prior human or bot reviews on this PR beyond a CodeRabbit rate-limit notice.
This branch has not been deployed
2 participants
Problem
BUN_CONFIG_NO_VERIFY=0turns tarball integrity verification off: a tarball whose bytes do not match the sha512 advertised by the registry installs with exit code 0 and the advertised hash is written tobun.lock.BUN_CONFIG_NO_VERIFY=1(or any other value) leaves verification on, so the variable cannot be used for what its name says.src/install/PackageManager/PackageManagerOptions.rs:672setsDo::VERIFY_INTEGRITYtovalue != "0", the opposite of theBUN_CONFIG_SKIP_*variables handled a few lines above it (value == "0"). The comparison has been inverted since the variable was added alongside--no-verifyin b897ad3 (2022) and was ported as is.--no-verifyitself is unaffected.Fix
PackageManagerOptions.rs:672:check_bool != b"0"becomescheck_bool == b"0", soBUN_CONFIG_NO_VERIFY=<anything but 0>skips verification (same as--no-verify) andBUN_CONFIG_NO_VERIFY=0keeps it on, matching howBUN_CONFIG_SKIP_SAVE_LOCKFILE,BUN_CONFIG_SKIP_LOAD_LOCKFILEandBUN_CONFIG_SKIP_INSTALL_PACKAGESare read right above it. This is the only line of behavior change.--no-verifystill wins when both are given; it is applied after the env var, unchanged.BUN_CONFIG_SKIP_*ones), and this is its only reference in the repo, so nothing in CI or tests depended on the inverted reading.test/cli/install/bun-install-tarball-integrity.test.ts, three new cases in the existing "tarball integrity metadata forms" block. The registry advertises the sha512 of a different tarball than the one it serves:BUN_CONFIG_NO_VERIFY=1 skips the integrity check like --no-verify: fails on main (main printsIntegrity check failed), passes with this change.BUN_CONFIG_NO_VERIFY=0 keeps the integrity check enabled: fails on main (main installs the package), passes with this change.--no-verify installs a tarball whose bytes don't match the advertised integrity: passes on main and here; pins the behavior the env var now mirrors.Background
bun install, each registry tarball is hashed while it is extracted and compared against thedist.integrityvalue from the package manifest (or the lockfile); a mismatch is reported asIntegrity check failed for tarball: <name>and the install fails.--no-verifyturns that comparison off by clearing theDo::VERIFY_INTEGRITYoption bit;BUN_CONFIG_NO_VERIFYis the environment-variable form of the same switch.Dois the bitflags set of per-run actions inPackageManagerOptions.rs(save lockfile, install packages, verify integrity, ...). TheBUN_CONFIG_*environment variables are read inOptions::loadand are applied before the CLI flags, so flags override them.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The BUN_CONFIG_NO_VERIFY environment variable was read with inverted polarity in PackageManagerOptions.rs, assigning the result of a check_bool != "0" comparison to the VERIFY_INTEGRITY flag, so setting it to 1 kept tarball integrity verification on while setting it to 0 silently disabled it. The fix flips that comparison so the variable clears VERIFY_INTEGRITY when truthy and leaves it set when 0, matching the BUN_CONFIG_SKIP_* variables beside it and the documented --no-verify flag, which is still applied afterwards and keeps its precedence. Three new tests in bun-install-tarball-integrit…