install: keep git, folder, tarball and workspace entries as written on bun update <name> - #38827
install: keep git, folder, tarball and workspace entries as written on bun update <name>#38827robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 17 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 (3)
Comment |
|
Status: closed in favour of #38847 Reproduced on main (debug build, The fix here (widening the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a one-clause guard that stops bun update <name> --latest from converting non-registry entries (git, folder, tarball, workspace) into <name>@latest registry lookups.
What was reviewed:
- Confirmed
Tag::inferclassifiesgit+file:,file:,workspace:,link:as non-npm andnpm:<name>@…/ dist-tags as npm, so the new.is_npm()gate matches the barebun update --latestpath (edit_update_entries) exactly. - Verified
is_npm()istag < 3(Uninitialized/Npm/DistTag) andinfernever returns Uninitialized, so aliased and dist-tag entries still rewrite tolatest; thedep-with-tags→^3.0.0assertion covers this. - New tests cover the explicit-name form, the
"*"pattern form, and-rwhere a sibling workspace declares the same name from the registry and must still move — the gitno-depsentry is the case that silently swapped packages before.
Extended reasoning...
Overview
The PR touches one line of runtime code in src/install/PackageManager/PackageJSONEditor.rs: the pre-install rewrite in edit() that turns a named entry into latest under bun update <name> --latest now first checks dependency::Tag::infer(version_literal).is_npm(). The rest of the diff is three new test cases plus supporting helpers (gitRepo, setupNonRegistryDeps) in test/cli/install/bun-update-lockfile-sync.test.ts.
Correctness
I traced Tag::infer (src/install/dependency.rs:753) for every literal shape used in the tests: git+file: → Git, file:./dir → Folder, file:./x.tgz → Tarball, workspace:* → Workspace, pre-2 → DistTag, ^1.0.0 → Npm, npm:name@~1.0.0 → recurses to Npm. is_npm() is (tag as u8) < 3, i.e. Uninitialized/Npm/DistTag; infer never returns Uninitialized. So the guard admits exactly the entries the registry answers and skips the rest — the same predicate the bare --latest path in edit_update_entries (same file, ~line 490) and the -r resolver in PackageManagerEnqueue.rs already use. The named and bare forms now agree, which is the stated goal.
The gate is applied to version_literal after with_alias_of re-attaches any npm:<name>@ prefix from the existing entry, so aliased npm entries (npm:no-deps@~1.0.0) still infer as Npm and are rewritten to npm:no-deps@latest. The existing aliased --latest test row and the new dep-with-tags → ^3.0.0 assertion pin that behavior. update_to_latest is only true when subcommand == Update, so bun add is unaffected.
Security risks
None. This narrows a rewrite; it cannot cause a package that was not previously fetched to be fetched. If anything it removes a supply-chain footgun (silently swapping a git dependency for the registry package of the same name).
Level of scrutiny
Low-to-medium: a single boolean added to an existing conditional in the package manager, mirroring an established sibling check in the same file. No new unsafe, no allocation/lifetime changes, no cross-platform branching. The tests are hermetic (local Verdaccio, git+file:// repo built in the temp dir with GIT_CONFIG_NOSYSTEM=1), assert exact package.json / bun.lock contents, and include the positive case (registry-declared workspace still moves under -r --latest).
Other factors
No prior human review comments; the only timeline entry is a CodeRabbit rate-limit notice. No CODEOWNERS covers src/install/. The bug-hunting pass found nothing. Test helpers follow the file's existing conventions (setup, run, expectInSync, describe.concurrent).
1 similar comment
|
Updated 7:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit c2f2216 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38827That installs a local version of the PR into your bun-38827 --bun |
There was a problem hiding this comment.
LGTM — one-line guard bringing the named --latest path in line with the bare path.
What was reviewed
- Confirmed
Tag::is_npm()covers exactlyNpm+DistTag(tags 0-2;infernever returnsUninitialized), so aliasednpm:pkg@…literals still rewrite while git/folder/tarball/workspace stay put. - Checked the guard placement: it runs after
with_alias_of, so an aliased npm entry is still classified as npm and moved. - Tests cover explicit names, the
"*"expansion, and the-rmixed case (git stays, registry sibling moves), with strict-equal assertions on package.json, lockfile, installed version, and a frozen-lockfile round-trip.
Extended reasoning...
Overview
One-line change to PackageJSONEditor::edit (src/install/PackageManager/PackageJSONEditor.rs:1372): the before-install rewrite of a named entry to latest under bun update <name> --latest is now gated on dependency::Tag::infer(version_literal).is_npm(). Three new test cases in test/cli/install/bun-update-lockfile-sync.test.ts plus gitRepo/setupNonRegistryDeps helpers.
Security risks
None. This narrows when a rewrite happens; no new I/O, allocation, or untrusted-input parsing. The git repo in tests is local (git+file://) with GIT_CONFIG_NOSYSTEM=1, explicit author/committer env, and --no-gpg-sign for hermeticity.
Level of scrutiny
Low-to-medium. The fix is a strict narrowing that only makes --latest more conservative — non-registry literals are now left as written instead of being swapped for a registry lookup of the same name. The predicate and the pattern are copied from the sibling edit_update_entries path (which already gates on tag != Npm && (tag != DistTag || !update_to_latest)), and the PR description enumerates the other --latest paths (bare update, -r/--filter resolver in PackageManagerEnqueue.rs) that already had the check. I verified is_npm() is (self as u8) < 3 over Uninitialized=0/Npm=1/DistTag=2, and that the guard sits after with_alias_of so npm:name@range aliases are still classified as npm and rewritten.
Other factors
Test construction is solid: no-deps is declared from a local git repo while the Verdaccio registry has no-deps@2.0.0, so on main the assertion expect(await resolutions(dir, "no-deps")).toStrictEqual([gitResolution]) fails loudly (the git entry becomes ^2.0.0), and the folder/tarball entries 404 — the test proves the mechanism, not just absence of crash. The -r case additionally asserts the registry-declaring workspace still moves to ~2.0.0, so the guard doesn't over-suppress. dep-with-tags: pre-2 is the positive control that dist-tags still rewrite to ^3.0.0. All assertions are toStrictEqual on the full dependencies object plus expectInSync (frozen-lockfile). Tests follow harness conventions (await using, bunEnv spread, concurrent describe, no public network). No CODEOWNERS on this path.
…n bun update <name> A named update rewrote whatever the package.json entry held when the command carried --latest or an explicit <name>@<spec>: a git, file:, tarball or workspace: entry was replaced with `latest` or the requested range before the install, resolved from the registry, and written back as a range of the unrelated registry package of that name (or the command failed with a 404 when the registry has no such name). The write-back after the install also turned a `workspace:^` entry into `workspace:*`. The editor already skips `catalog:` entries in both passes; widen that check to every entry kind the registry does not resolve, so such entries are only re-resolved, like the bare update already treats them.
45c929c to
c2f2216
Compare
robobun
changed the title
There was a problem hiding this comment.
LGTM — widens the existing per-entry catalog: skip in PackageJSONEditor::edit to !Tag::infer(entry).is_npm(), so bun update <name> no longer converts git/folder/tarball/workspace: entries into registry ranges.
What was reviewed
- Confirmed
Tag::is_npm()={Npm, DistTag}on inferred tags (empty string →DistTag), so freshly-created empty slots andnpm:alias@…entries still fall through, and thedep-with-tagscontrol row proves dist-tags are still rewritten by--latest. - Checked the earlier
keep_catalog_referenceguard — it's already gated onsubcommand != Update, so widening it is unnecessary. - Verified this brings the named path in line with
edit_update_entries(barebun update --latest), which already gates ontag != Npm && (tag != DistTag || !--latest). - Test fixture covers
--latest,"*" --latest, plain names,@latest,@^2, and-rwith a sibling workspace that must still move; assertions check package.json, bun.lock, resolutions, and installed contents.
Extended reasoning...
Overview
The PR changes one predicate in src/install/PackageManager/PackageJSONEditor.rs (the final write-back loop of edit()): the existing bun update skip for catalog: entries becomes a skip for every entry whose inferred tag is not Npm/DistTag. This runs in both the before-install and after-install passes, so a git/github/folder/tarball/symlink/workspace: entry is now left byte-identical however it is named — with --latest, with an explicit @spec, via a pattern, or via -r/--filter. One doc line and ~100 lines of tests accompany the two-line code change.
Security risks
None introduced. The old behaviour was arguably the riskier one: bun update <name> --latest on a git entry silently swapped it for whatever registry package shares the name. The fix narrows what the registry is consulted for.
Level of scrutiny
Moderate: this is a user-visible behaviour change in bun update, but it is a targeted correctness fix that aligns the named-update path with what the bare bun update --latest editor and the -r/--filter resolver already do (both already restrict themselves to npm ranges/dist-tags). The design question — silently keep the entry vs. error — is answered by precedent: catalog: already behaves this way, npm/pnpm do not convert such entries, and the entry's row is still re-resolved during the install. The PR description explicitly reasons through each of these.
Other factors
Tag::infernever returnsUninitialized(empty →DistTag), sois_npm()on an inferred tag is exactlyNpm || DistTag; freshly-allocatedb""slots still pass the check.- The now-unreachable
resolution::Tag::Workspace => b"workspace:*"write-back arm is intentional: the PR notes #38847 handles the remaining workspace cases, and underbun addthat arm is still live. - Tests are hermetic (Verdaccio + local
git initundergitEnvwithGIT_CONFIG_NOSYSTEM=1), assert exacttoStrictEqualshapes on both files plus lockfile resolutions and installednode_modules, and include a positive control (dep-with-tags) and a mixed-rcase where one workspace's registry entry must still move. - No CODEOWNERS cover the touched paths; no prior human review comments to address.
|
Heads-up on overlap: #38847 (named update of workspace member entries) now skips every non-registry entry at the top of the write-back loop in |
1 participant
Problem
bun update <name> --lateston a dependency declared from git, e.g."repo-a": "git+file:///.../repo-a#v1.0.0", exits 0 and leaves package.json with"repo-a": "^1.0.1"and bun.lock with the registry packagerepo-a@1.0.1: the git dependency is silently replaced by the unrelated registry package of the same name. For a name the registry does not have (afile:folder, a local tarball) the command fails witherror: GET <registry>/<name> - 404instead.bun update <name>@latestandbun update <name>@^2on such an entry do the same thing (with@latestthe entry is left as the literal string"latest"), andbun update "*" --latest,bun update --dev --latestandbun update <name> -r/--filter --latestexpand to the same path.PackageJSONEditor::edit(src/install/PackageManager/PackageJSONEditor.rs) rewrites the entry before the install without looking at what kind of literal it holds. It writeslatestunder--latestand the requested spec under<name>@<spec>, the install resolves that from the registry, and the second pass pins what was resolved. The only kind it already refused to touch wascatalog:(line 1340), in both passes.bun update --latesteditor (edit_update_entries, same file, line 490) and the-r/--filterresolver (src/install/PackageManager/PackageManagerEnqueue.rs:2449) already restrict themselves to npm ranges and dist-tags; the named editor was the one path without that check.Fix
catalog:check to every entry kind the registry does not resolve:!Tag::infer(entry).is_npm(), i.e. anything other than an npm range or a dist-tag (catalog:,workspace:, git, github, folder, tarball,link:). Underbun updatesuch an entry is now left as written by both passes, whatever was requested for it;bun addis unaffected and still replaces it.bun update <name>already did for it and what the barebun update --latestdoes for the same entry.--latestand<name>@<range>both mean "move within the registry", which has no meaning for an entry the registry does not resolve; npm and pnpm do not convert such entries into registry ranges either, and this is already the documented behaviour ofbun updateforcatalog:references (tested with an explicit spec in test/cli/install/bun-add-catalog.test.ts), so the same rule now covers every kind.workspace:*arm is no longer reached for aworkspace:^/workspace:~entry named inbun update. install: keep workspace and other non-registry entries as written on bun update <name> #38847 fixes the remaining workspace cases (a plain range that links a member, requests bound to the root's implicit member rows); its widening of this same check is subsumed by this change, the rest of it is not.catalog:one.-rcase in test/cli/install/bun-update-lockfile-sync.test.ts: explicit names with--latest,"*" --latest, plain names,no-deps@latest,no-deps@^2.0.0, andno-deps -r --latestwhere another workspace declares the same name from the registry and must still move. The fixture declaresno-depsfrom a local git repository while the registry hasno-deps@2.0.0, plusfile:folder and tarball entries and aworkspace:^member; with the editor from main all six fail (the git entry becomes^2.0.0/latest,file:entries 404,workspace:^becomesworkspace:*), with this change all six pass.Background
bun update <name>edits package.json in two passes around the install: before the install,edit()may rewrite the named entries to what should be resolved (latestunder--latest, the spec given as<name>@<spec>); the install resolves whatever package.json now says; after the install,edit()runs again and pins the resolved version in the declared range style. Patterns, the--dev/--prodselectors and-r/--filterall feed this same function.dependency::Tag::inferclassifies a version literal:^1.0.0isNpm,pre-2isDistTag,npm:foo@^1follows the part after the alias,git+.../github:areGit/Github,file:...isFolderorTarball,workspace:*isWorkspace,catalog:isCatalog.Tag::is_npm()is true for exactly the two kinds a registry manifest answers, and is the predicate the resolver uses to decide whether to fetch one.Earlier revision of this PR
The first revision only gated the
--latestrewrite (theif update_to_latestbranch further down inedit()) onis_npm(). Self-review pointed out that the same entries were still converted bybun update <name>@latest/<name>@<range>, and that the second pass still rewroteworkspace:^toworkspace:*, because the rule belongs on the per-entry check that already existed forcatalog:and runs in both passes. The current revision moves it there and adds the explicit-spec and plain-name rows to the test.