Conversation
…lassified or re-parsed Dependency::parse strips leading whitespace from a package.json version literal before inferring its tag, but the package.json editor, the string buffer pre-count in Package::parse, the workspace: alias check and Dependency::clone all looked at the raw literal. Route them through one helper so a literal like " npm:no-deps@^1.0.0" behaves the same as the trimmed value everywhere: bun update keeps the alias and updates the entry, " catalog:" references survive bun update <name>, a " file:" folder dependency no longer panics on install or gets saved to the lockfile with an empty specifier, and " workspace:name@range" resolves.
|
Warning Review limit reached
Next review available in: 30 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 (10)
Comment |
|
Status: reproduced and fixed in this PR.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 1:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit a7d2c38 has some failures in 🧪 To try this PR locally: bunx bun-pr 38309That installs a local version of the PR into your bun-38309 --bun |
preserve_version_prefix (bun update --interactive) and edit_root_package_json (bun pm pack / publish) also read the raw package.json value, so a leading space dropped the npm: alias and the range prefix on write-back, and left a workspace:/catalog: protocol in the packed package.json.
There was a problem hiding this comment.
Both prior findings are now addressed — a7d2c38 trims the literal in preserve_version_prefix (bun update -i) and edit_root_package_json (bun pm pack), with tests for each in bun-install-registry.test.ts, bun-pack.test.ts, and catalogs.test.ts. This pass found nothing new. Given the number of package-manager entry points touched (install, update, update -i, pack, both lockfile formats' round-trip, workspace resolution), a human look is still worthwhile.
Checked this round: the trimmed slice is only used for classification/parsing while sliced.value() (the stored literal) stays untrimmed, so bun.lock output is unchanged for existing lockfiles; trim_literal is the identity on values without leading whitespace; the Windows-only Tag::infer in parse_dependency and the string-buffer capacity pre-count both see the same bytes parse does.
Extended reasoning...
Overview
Introduces dependency::trim_literal (a trim_left over " \\t\\n\\r") and applies it at every site that classifies or re-parses a package.json version literal on its own: parse / parse_with_optional_tag (no behavior change — they already did this inline), clone_with_different_buffers and Version::to_version (lockfile save/load re-parse), the string-buffer capacity pre-count and the workspace: prefix check in Package::parse_dependency, the Windows-only path conversion there, seven Tag::infer calls plus three alias write-back sites in PackageJSONEditor.rs, preserve_version_prefix in update_interactive_command.rs, and edit_root_package_json in pack_command.rs. Five test files add coverage across bun update (bare / <name> / --latest / --interactive), bun install with both lockfile formats, bun pm pack, catalog references, and workspace aliases.
Security risks
None identified. The change narrows what bytes are handed to existing classifiers; it does not introduce new parsing, filesystem paths, or network surface. Trimming leading whitespace before a starts_with / first-byte check cannot enable a value that was previously rejected — parse already accepted these values, which is the whole premise.
Level of scrutiny
High. This is core package-manager code: lockfile serialization/deserialization, package.json rewriting across four commands, and workspace resolution. A wrong classification here corrupts a user's package.json or lockfile silently. The pattern applied is mechanical and the helper is the identity for the overwhelmingly common case (no leading whitespace), which bounds the blast radius, but the number of independent entry points and the two real siblings found across earlier review rounds argue for a human pass.
Other factors
Two earlier automated review rounds each found a missed sibling (to_version for bun.lockb; update -i and bun pm pack); both were fixed with dedicated tests in follow-up commits. The PR description enumerates every touched call site with the specific failure each one produced and documents which test isolates each line. Test coverage is a genuine matrix (9 value×command combinations, both lockfile formats round-tripped with --frozen-lockfile, workspace pass/fail sets, catalog under four update forms, pack for both workspace: and catalog:). The stored version.literal is deliberately kept untrimmed (asserted in the bun.lock test), so existing lockfiles do not churn.
This branch has not been deployed
1 participant
Problem
"aliased-dep": " npm:no-deps@^1.0.0", installs fine, butbun update aliased-deprewrites it to"^1.1.0": the alias target is gone and the next install looks for a registry package calledaliased-dep. A barebun update(with or without--latest) leaves such an entry untouched in package.json while node_modules moves," ^1.0.0"is skipped by a barebun update, andbun update <name>replaces a" catalog:"reference with a concrete version (the same command keeps"catalog:").Dependency::parsestrips leading whitespace before callingTag::infer(src/install/dependency.rs,parse/parse_with_optional_tag), so the installer accepts these values. The package.json editor classified the raw literal instead (src/install/PackageManager/PackageJSONEditor.rs: the before-install loop ofedit_update_no_args_in,edit_catalogs_before_update, the'add_packages_to_updateblock and bothcatalog:checks inedit).Tag::inferswitches on the first byte, so" npm:..."is not seen as an alias or a range and" catalog:"is not seen as a catalog reference; the entry is never recorded and the post-install write-back falls through to the code that writes a bare^<version>.src/install/lockfile/Package.rspre-counts string buffer capacity withTag::infer(value)and only reserves room for a folder path when the value classifies as one." file:./vendor/some-long-directory-name/lib"is not classified as a folder there but is parsed as one a few lines later, sobun installdies withpanic: range end index 78 out of range for slice of length 43inStringBuilder::append_with_hash(lockfile.rs), called fromPackage::parse_dependency.Dependency::clone_with_different_buffers(src/install/dependency.rs) re-parses the stored literal withparse_with_tagwithout trimming. For" file:./pkg"the folder arm rejects" file"as a protocol and the clone becomes an empty version, so the first install writes"pkg": ""to bun.lock and the second one fails witherror: pkg@ failed to resolve. For" catalog:"the catalog arm hitsdebug_assert!(dependency.starts_with(b"catalog:"))in debug builds; in release builds it reads the group name as:andbun updatefails withfailed to resolve. Every lockfile save goes through this clone.Version::to_version, which loads a dependency frombun.lockb, re-parses the stored literal the same way, so with a binary lockfile the second install of" file:./vendor/..."fails witherror: Unsupported protocol file:./vendor/....Package::parse_dependencycheckshas_prefix(sliced.slice, b"workspace:")on the raw literal, so" workspace:@org/b@*"is looked up under the dependency's own name and fails withWorkspace dependency "a1" not found, and" workspace:@org/b@1.0.0"skips the version check.preserve_version_prefixinsrc/runtime/cli/update_interactive_command.rs(bun update -i) writes" npm:no-deps@^1.0.0"back as"2.0.0"and"\t~1.0.0"as"2.0.0", dropping the alias target and the range prefix, andedit_root_package_jsoninsrc/runtime/cli/pack_command.rs(bun pm pack,bun publish) leaves" workspace:^"and" catalog:"in the packed package.json instead of replacing them with versions.Fix
dependency::trim_literal(src/install/dependency.rs), the stripparsewas already doing, and uses it at every place that classifies or re-parses a literal on its own:parseandparse_with_optional_tag(no behavior change, they now share the definition),clone_with_different_buffers,to_version, the capacity pre-count, the Windows-only path conversion inparse_dependency(the sameTag::infer(version)call three lines into the function; type-checked withcargo check -p bun_install --target x86_64-pc-windows-msvc, its conversion is a no-op for the forward-slash path the new test uses), theworkspace:prefix check, the sixTag::infercalls in the editor, and the two CLI rewrites (preserve_version_prefix,edit_root_package_json), which import the same helper frombun_installrather than carrying their own copy of the rule.npm:<name>prefix from when it writes the new version back (editandedit_catalogs_after_updateuse the recorded literal,edit_update_no_args_inuses the lockfile's), so all three paths writenpm:no-deps@^1.1.0, the same bytes they write for the value without the whitespace. Plain ranges were already rewritten from scratch (" ^1.0.0"became"^2.0.0"under--latestbefore this change), so this makes the alias case consistent with them.parsedefines what a literal means, and it has ignored leading\t\n\rsince the Zig implementation. Every site touched here is making a decision about the same literalparseaccepts, so it has to look at the same bytes; otherwise the editor, the capacity count, and the clone disagree with the install they are describing. Only the bytes being classified change:Dependency.version.literal, the lockfile text, andoriginal_version_literalare stored exactly as before (the folder test checks that bun.lock keeps the literal with its space), so existing lockfiles do not churn. Literals without leading whitespace are byte-for-byte unaffected becausetrim_literalis the identity on them.parse_with_tagarms that copy the raw literal into the value itself (" latest"looks up a dist-tag named" latest", a bare" ./pkg"folder and a" ./x.tgz"local tarball keep the space in their path). That is a different shape of fix inside one function and is tracked separately. The remaining prefix checks found by grepping fornpm:/workspace:/catalog:/file:(bun why,bun audit, the yarn/pnpm/package-lock migrations,Resolutionparsing) look at resolution strings or other package managers' lockfiles, not package.json literals, and are left alone.bun update <alias> --latest404s regardless of whitespace (install: keep npm: alias targets when bun update rewrites versions #38224), so it is not in the matrix here.src/stashed, passing with it:test/cli/install/bun-install-registry.test.ts(update > leading whitespace in the version literal): 9 value/command combinations over^and~aliases and plain ranges with a space, tab and newline, crossed withbun update,bun update <name>andbun update --latest(7 of them fail before, 2 document that the already-working combinations keep working), plus a package listed in two dependency groups where the" catalog:"entry in the earlier group must survive (this is the only test that fails if the post-installcatalog:check inedit_update_no_args_inis left untrimmed; verified by reverting that one line). Abun update --interactive --latestrun driven through stdin (a, enter) checks that the same two literals come back asnpm:no-deps@^2.0.0and~2.0.0(before:2.0.0for both).test/cli/install/catalogs.test.ts(update): catalog entries with leading whitespace underbun updateandbun update --latest; a" catalog:"reference kept bybun update <pkg>andbun update <pkg> --latest. These also exercise the clone fix: without it the debug build asserts duringbun install.test/cli/install/bun-pack.test.ts:" workspace:^"added to the existing protocol-replacement table (expects^1.1.1);test/cli/install/catalogs.test.ts(pack): a" catalog:"reference is replaced with the catalog's version in the packed package.json. Both shipped the protocol verbatim before.test/cli/install/bun-install.test.ts: a" file:"dependency with a path longer than an inline string installs (the panic), andbun install --frozen-lockfilefrom the lockfile it wrote works; run once with bun.lock (which must keep the literal as written; the second pass covers the clone) and once with bun.lockb (the second pass coversto_version, and failed with theUnsupported protocolerror until that call was trimmed).test/cli/install/bun-workspaces.test.ts(workspace aliases):" workspace:@org/b@*"added to the passing set," workspace:@org/b@1.0.0"to the set that must fail withNo matching version.bun-install-registry.test.ts(240 pass, 5 pre-existing todo),catalogs.test.ts,bun-workspaces.test.ts,bun-update.test.ts,bun-add.test.ts,bun-lock.test.ts,bun-lockb.test.ts,overrides.test.ts,bun-pack.test.ts, the threetest/cli/update_interactive_*.test.tsfiles, andbun-install.test.ts(the 14 failures there are the bitbucket/gitlab/public-URL tests, which fail identically with the released binary in this sandbox because it has no network).cargo clippy -p bun_install --no-deps,cargo check -p bun_runtimeandcargo fmt --checkare clean.Background
"^1.0.0","npm:real-name@^1.0.0","file:./dir","catalog:","workspace:name@range").Tag::inferclassifies one by looking at its first byte and prefix;parseturns it into aDependency.version, keeping the original text asversion.literal, which is what bun.lock prints and what the editor reads back.npm:<real-name>@<range>; the key is a local name that need not exist in the registry, so writing the range back without thenpm:<real-name>@prefix changes which package is installed.bun updateedits package.json: before the install it records the entries it intends to update (and, under--latest, swaps them for alatestplaceholder in memory); after the install it writes the resolved versions back in the original pin style. An entry that was not recorded is either left alone or, forbun update <name>, written as a bare range from the request itself, which is how the alias was lost.Package::parsefirst walks the entries to count how many bytes it will append, then appends; a folder dependency additionally appends its resolved relative path, so the count reservesMAX_PATH_BYTESfor anything that classifies as a folder. Appending more than was counted is the slice panic above.clone_with_different_bufferscopies the literal and re-parses it with the tag it already has. Loading abun.lockb(to_version) re-parses stored literals the same way. These two re-parses are the only places besidesparsethat turn literal bytes into a version, which is why they need the same trim; the otherparse_with_tagcallers hand it a workspace path, a package's ownversionfield, or a package-lockresolvedURL, none of which is a package.json literal.Behavior on the current release vs this branch (local test registry; no-deps has 1.0.0, 1.0.1, 1.1.0, 2.0.0)
" npm:no-deps@^1.0.0"update aliased-dep^1.1.0(alias lost)npm:no-deps@^1.1.0" npm:no-deps@^1.0.0"updatenpm:no-deps@^1.1.0" npm:no-deps@^1.0.0"update --latestnpm:no-deps@^2.0.0"\tnpm:no-deps@~1.0.0"updatenpm:no-deps@~1.0.1" ^1.0.0"update^1.1.0" ^1.0.0"update no-deps/update --latest^1.1.0/^2.0.0(already worked)" catalog:"in a workspace packageupdate no-deps^1.1.0(reference lost)" ^1.0.0"update^1.1.0"\tnpm:no-deps@~1.0.0"update"\tnpm:no-deps@~1.0.1"npm:no-deps@~1.0.1" file:./vendor/some-long-directory-name/lib"installpanic: range end index 78 out of range for slice of length 43" file:./pkg"installtwiceerror: pkg@ failed to resolve(bun.lock has"pkg": "")" file:./vendor/..."with bun.lockbinstalltwiceerror: Unsupported protocol file:./vendor/..." workspace:@org/b@*"installWorkspace dependency "a1" not found" npm:no-deps@^1.0.0","\t~1.0.0"update -i --latest2.0.0,2.0.0npm:no-deps@^2.0.0,~2.0.0" workspace:^"pm pack" workspace:^"^1.1.1" catalog:"pm pack" catalog:"^1.0.0(the catalog entry)