Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 26 days. After that, they cost $0.25 per reviewed file. Or wait 29 seconds 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 selected for processing (2)
Comment |
There was a problem hiding this comment.
LGTM — one-prefix addition so Tag::infer recognizes the git+git:// spelling that bun.lock itself writes.
What was reviewed:
- Traced
parse_with_tag'sTag::Gitarm: it strips the leadinggit+, so the repo string (git://host/...) matches what's parsed from package.json. - Checked that
git://github.com/...deps take theb':'branch →Tag::Githuband are never written asgit+git://, so the new arm skipping the github.com check is fine. - Test mirrors the existing "git hosts round-trip" test right above it (same
synthetic+migrate+frozenpattern, offline,--lockfile-only).
Extended reasoning...
Overview
Adds b"+git:" to the list of git+ sub-schemes that Tag::infer classifies as Tag::Git in src/install/dependency.rs, alongside the existing +ssh: and +file:. This closes the round-trip gap where bun.lock writes a git:// dependency's resolution as git+git://host/repo#sha but the reader then falls through to DistTag → UnexpectedResolution. A new test in migrate.test.ts migrates a package-lock.json with two git:// entries, asserts the git+git:// rows in bun.lock, and runs --frozen-lockfile --lockfile-only to prove the lockfile parses.
Security risks
None. This only widens which prefix strings are classified as Git dependencies; the downstream Git handling (parse_with_tag's Tag::Git arm, Repository::parse_append_git) already strips git+ and handles arbitrary Git URLs. No new input surface, no path handling, no auth.
Level of scrutiny
Low. The Rust change is a single || clause added to an existing condition, byte-for-byte matching the pattern of its two siblings. The PR description traces the exact write path (repo.fmt("git+", ...)) that produces this string and the read path (from_text_lockfile → Tag::infer) that failed on it. I verified that git://github.com/... dependencies are classified as Tag::Github at write time (via the b':' branch), so they never reach bun.lock as git+git:// and the new arm doesn't need a github.com special case.
Other factors
The test is a near-copy of the adjacent "git hosts round-trip (B1, github: parity)" test — same synthetic() helper (offline registry on port 1), same migrate() + frozen() pattern, so it's hermetic and --lockfile-only avoids any actual clone of example.com. It covers both a bare git:// URL and one with a #v1 ref. The bug-hunting system found nothing. No prior reviewer comments to address.
|
Updated 5:35 PM PT - Aug 25th, 2026
❌ @robobun, your commit d8b7a9c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38798That installs a local version of the PR into your bun-38798 --bun |
|
CI status: the branch is rebased on main (the conflict was two tests appended at the same spot in The diff itself is unchanged since review: one added |
A dependency on a plain git:// URL installs, but its resolution is written to bun.lock as git+git://host/repo#sha and Tag::infer did not recognize that prefix, so every following install rejected the lockfile with "Unexpected resolution" and --frozen-lockfile could never pass. Accept git+git: alongside git+ssh: and git+file:.
c17c4f7 to
22dab07
Compare
There was a problem hiding this comment.
LGTM — one-line classifier fix with a hermetic round-trip test.
Checked that Tag::infer's git+ branch now mirrors the +ssh:/+file: arms exactly and that starts_with(b"+git:") is the byte-compare form src/CLAUDE.md allows. Verified the test's migrate (bun pm migrate) and frozen (--frozen-lockfile --lockfile-only) helpers never fetch git://example.com, so the test is offline. Test sits next to the existing "git hosts round-trip" case and reuses synthetic/npmLock/sha — no new helpers or fixtures. No CODEOWNERS cover these paths.
Extended reasoning...
Overview
The PR adds || url.starts_with(b"+git:") to the git+ suffix match in Tag::infer (src/install/dependency.rs:812), so that git+git://... — the form bun.lock's writer emits for a plain git:// dependency — is classified as Tag::Git instead of falling through to DistTag and failing lockfile parsing. A concurrent test in test/cli/install/migration/migrate.test.ts synthesizes an npm package-lock with two git:// entries, migrates it, asserts the git+git://...#sha rows in bun.lock, and re-runs --frozen-lockfile --lockfile-only to prove the round-trip.
Security risks
None. This is a string-prefix classifier for dependency resolution tags; the added arm returns the same Tag::Git as its +ssh:/+file: siblings and does not touch any network, filesystem, or auth path. The test uses example.com hosts but only through bun pm migrate and --lockfile-only, which never clone.
Level of scrutiny
Low. The Rust change is a single disjunct added to an existing conditional, byte-for-byte matching the neighboring pattern. The comment is one line and explains the non-obvious git+git:// doubling. [u8]::starts_with is explicitly allowed by src/CLAUDE.md. The test reuses the file's established synthetic/npmLock/migrate/frozen/sha helpers, is test.concurrent, uses tempDir via synthetic, and asserts with toStrictEqual/toBe before the exit-code check inside frozen.
Other factors
The bug hunt exited on dry_streak with no findings and no ruled-out candidates. CODEOWNERS does not cover src/install/ or this test file. The earlier bot inline comment at dependency.rs:810 was followed by commit f0df2b0 ("shorten the git+git comment"), which plausibly addressed it; there is no outstanding CHANGES_REQUESTED from a human reviewer. The test lives immediately beside the existing "git hosts round-trip (B1, github: parity)" case, satisfying the "add to the existing test file" rule.
Problem
git://URL (whatgit daemonserves, e.g."repo-a": "git://127.0.0.1:9418/repo-a#v1.0.0") installs, but every install after that one printserror: Unexpected resolution: git+git://127.0.0.1:9418/repo-a#49b7cef.../InvalidLockfile: failed to parse lockfile: 'bun.lock', warnsIgnoring lockfile, and re-resolves from scratch.bun install --frozen-lockfilealways fails withlockfile had changes, but lockfile is frozen, andbun pm ls,bun dedupeandbun pruneexit 1 on the same parse error. The same happens to abun.lockmigrated from apackage-lock.jsonthat has a"resolved": "git://..."entry (the form npm 11 writes).bun.lockwrites every Git resolution as thegit+label followed by the repository URL as written (src/install/lockfile/bun.lock.rs,repo.fmt("git+", ...)), so agit://dependency becomesgit+git://host/repo#sha. When the lockfile is read back,Resolution::from_text_lockfile(src/install/resolution.rs) classifies that string withTag::infer(src/install/dependency.rs), whosegit+branch only knowsgit+ssh:,git+file:andgit+http(s):.git+git:falls through toDistTag, whichfrom_text_lockfilereports asUnexpectedResolution.git://dependencies did not install at all there (no commit matching ...); on current main the clone succeeds, so the unreadable lockfile is what users hit.Fix
Tag::infertreatsgit+git:likegit+ssh:andgit+file:, returningTag::Git.Repository::parse_append_gitalready strips thegit+label, so the repository stored from the lockfile (git://host/repo) is identical to the one parsed frompackage.json, and the existing package lookup matches it.git+git://entries become readable without being rewritten.test/cli/install/migration/migrate.test.ts, "git:// hosts round-trip through bun.lock". It migrates apackage-lock.jsonwith twogit://entries (no network:bun pm migrateonly writesbun.lock), asserts thegit+git://rows bun writes, then runsbun install --frozen-lockfile --lockfile-only, which fails with the error above without the fix and passes with it.git daemon:bun installtwice (second one byte-identical, "no changes"),--frozen-lockfilewith and withoutnode_modules,--linker=isolated --frozen-lockfile,bun pm ls,bun dedupe --check,bun prune --dry-run, all clean with the fix;--frozen-lockfilefailed as described before it.bun bd testontest/cli/install/migration/migrate.test.ts,hosted-git-info/,bun-install-git-deps.test.ts,bun-lock.test.tspasses; the git-related cases ofbun-install.test.ts/bun-add.test.tspass except the bitbucket/gitlab ones, which need network access and fail identically without this change.Background
bun.lockstores each package as"<name>@<resolution>". For git packages the resolution is the clone URL plus#<commit>, prefixed withgit+for plain git remotes orgithub:for GitHub tarball installs. The prefix is what lets the reader tell the resolution kinds apart.Tag::inferis the one classifier used both for version strings inpackage.json(^1.0.0,github:a/b,git+ssh://...) and, viafrom_text_lockfile, for resolution strings read frombun.lock. Anything the lockfile writer can emit therefore has to be a shapeinferrecognizes;git+git://was the only prefix the writer produces that it did not.git://URL is kept verbatim as the repository string (only a leadinggit+is stripped), which is why the label ends up doubled rather than replacing the scheme.no test proof · iteration 5 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/migration/migrate.test.ts