Repository navigation
install: sort workspace deps by resolved name when writing bun.lock - #40809
Conversation
A git, github, or tarball dependency added with no alias enters the lockfile buffer under its version literal because the real package name is unknown until the source is fetched. assign_resolution later rewrites the name in place without a re-sort, so the workspaces section wrote the resolved key at the literal's sorted position. Sort the dependency ids by their current name at write time, the same way the packages section already does. Fixes #40803
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 15 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Comment |
|
Updated 10:00 AM PT - Aug 28th, 2026
❌ @robobun, your commit f1d027f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40809That installs a local version of the PR into your bun-40809 --bun |
The migration path inserts workspace dependencies in yarn.lock order. The lockfile writer now sorts them by name, so the snapshots change to the alphabetized order.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the write-time index sort mirrors what the packages section already does a few hundred lines up, and the two follow-up commits look to have addressed the earlier inline notes.
What was reviewed:
TreeDepsSortCtx::sortextraction is a byte-for-byte fold of the two existing closures; comparator unchanged.write_workspace_depsnow collectsdeps_list.begin()..end()into a freshVec<DependencyID>and sorts it — same pattern aspkg_deps_sort_bufat line ~770, so the shared buffer stays untouched and existing dep-id indices remain valid.- New test is hermetic (
git+file://viamakeSharedRepo,tempDir,bunExe/bunEnv), asserts stderr before exit code, and checks exact key order; yarn.lock migration snapshot reorderings are the expected fallout since that path never went through parse-time sort.
Extended reasoning...
Overview
This PR fixes issue #40803: bun install <git url> without an alias wrote the workspace dependency into bun.lock at the position of its version literal (e.g. git+file://...) rather than its resolved package name, because assign_resolution renames the dep in place after parse-time sorting. The fix adds a write-time index sort over the workspace's dependency-id range in write_workspace_deps (src/install/lockfile/bun.lock.rs), using the same TreeDepsSortCtx comparator the packages section already applies for the same reason. The two existing inline sort closures are folded into a TreeDepsSortCtx::sort() method with no behavior change. A new hermetic test in test/cli/install/bun-install-git-deps.test.ts sets up hhh-first/jjj-last file deps, installs a local bare-repo git dep resolving to iii-middle, and asserts the lockfile's workspace dependencies keys are alphabetical. Two yarn.lock migration snapshots are updated to reflect now-alphabetical workspace dep ordering.
Security risks
None. This is purely lockfile-serialization ordering; no untrusted-input parsing, no network, no auth/crypto. The test uses a local git+file:// repo and never contacts external hosts. The new Vec<DependencyID> is built from a bounded range already used elsewhere in the file (line ~770, ~3118, ~3178) and indexed into deps_buf the same way the pre-existing loop did.
Level of scrutiny
Low-to-moderate. The change is small (~30 net lines in bun.lock.rs), applies an established pattern from the same file to a sibling code path, and the comparator is unchanged so already-sorted lockfiles serialize identically. The only observable output change beyond the bug fix is yarn.lock migration ordering, which the snapshot updates capture and which is arguably a correctness improvement (previously it preserved yarn.lock's arbitrary order). No CODEOWNERS cover the touched paths.
Other factors
The prior run left two inline notes at lines 1332/1334; each was followed by a commit (7b15b9d and f1d027f) before the author resolved the thread, so they were plausibly addressed rather than dismissed — the current code at those lines is clean and matches file conventions. The new test follows harness conventions (tempDir, test.concurrent, bunExe/bunEnv via runInstall, stderr asserted before exit code, exact .toEqual on key order), is appended to the existing git-deps test file rather than a new regression file, and the PR body documents fail-before with USE_SYSTEM_BUN=1. The author also ran the broader lockfile/frozen-lockfile/migration suites. Exit reason was dry_streak, so the hunt ran to completion with no findings.
|
CI state on f1d027f: the new test and all lockfile suites pass. The remaining red lane is test/js/web/url/url.test.ts on darwin x64, which also fails on main and is not related to this change. The other failures passed on retry. |
2 participants
Problem
bun i github:user/repowrites the dependency key at the wrong position in theworkspacessection ofbun.lock. Afterbun i formidable handlebarsthenbun i github:lukeed/clsx, the rootdependenciesmap readsformidable, clsx, handlebars(issue Installing dependency from Github hash leads to incorrect alphabetization in lockfile #40803).github:lukeed/clsx) and is sorted there (src/install/lockfile/Package.rs:3056).assign_resolution(src/install/PackageManager/PackageManagerResolution.rs:267) later rewrites the name in place toclsxwithout a re-sort.write_workspace_depsthen iterated the stale buffer order.Fix
write_workspace_deps(src/install/lockfile/bun.lock.rs) now sorts the package's dependency ids by their current name at write time, with the sameTreeDepsSortCtxcomparator thepackagessection already uses for this exact reason.TreeDepsSortCtxsort closure are folded into onesortmethod. No behavior change there.test/cli/install/bun-install-git-deps.test.ts(new test, fails on stock bun). Also ran bun-lock, lockfile-only, bun-update-lockfile-sync, both frozen-lockfile suites, bun-add, bun-lockb, migrate-bun-lockb-v2, and yarn-lock-migration.Background
packagessection of the writer already builds a sorted id list per package for the same reason, so its output was never affected. Only theworkspacessection read the buffer directly.git:,github:, or tarball-URL add. The new test uses a localgit+file://repo, so it stays hermetic.Notes
dependencies,devDependencies,optionalDependencies,peerDependencies) are written by an outer loop that filters on behavior, so the write-time sort only needs name order. A name that appears in two groups is split by the filter, as in thepackagessection.package.jsonis edited with the resolved name (UpdateRequest::get_resolved_name), so a delete ofbun.lockplus a freshbun ialready produced the right order. Only the add path wrote the stale position, and a later no-change install preserved it. This matches the report.USE_SYSTEM_BUN=1 bun test test/cli/install/bun-install-git-deps.test.ts -t "sorts the workspace dependency"fails withiii-middlebeforehhh-first. Passes with the fix.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
When a git or tarball dependency is installed without an alias, the lockfile's workspace dependencies are sorted at parse time under the version literal (for example "github:user/repo"), and assign_resolution later rewrites the entry's name to the actual package name without triggering a re-sort, leaving the entry in the wrong alphabetical position. The fix re-sorts the dependencies by their current names at stringify time, so the lockfile is written using the final resolved names and the entries land in correct alphabetical order.