Skip to content

install: fix yarn.lock migration panic on tarball URLs with "/-/" right after the host - #38803

Open
robobun wants to merge 1 commit into
mainfrom
farm/7e42c7ee/yarn-migrate-registry-tarball-name
Open

install: fix yarn.lock migration panic on tarball URLs with "/-/" right after the host#38803
robobun wants to merge 1 commit into
mainfrom
farm/7e42c7ee/yarn-migrate-registry-tarball-name

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • bun install / bun pm migrate in a project whose yarn.lock has a tarball entry such as "x@https://evil.example/-/registry.npmjs.org/x.tgz" aborts instead of migrating or returning a migration error the way other bad lockfile input does (src/install/migration.rs:79-88):
    panic: slice index starts at 42 but ends at 20 (top frame bun_install::yarn::migrate_yarn_lockfile). Reproduces on the released canary (b7a0431) and on main.
  • Cause: the name_to_use block in migrate_yarn_lockfile (src/install/yarn.rs:942-970 on main) names a tarball package after its URL by running two unrelated substring searches over resolved, "/-/" and "registry.", and slicing between them. It assumes the host comes first; the only guard is that registry.npmjs.org/ or registry.yarnpkg.com/ occurs somewhere in the URL.
  • The same slice panics (27..26) whenever /-/ directly follows the registry host. The one real package this applies to is the npm package named - (tarball https://registry.npmjs.org/-/-/--0.0.1.tgz) declared as a URL dependency; every other shape needs a hand-edited yarn.lock, which bun install still has to survive. No issue reports this; it was found by reading the code.
  • Same code, no crash: https://registry.npmjs.org//-/z.tgz is written to bun.lock with an empty package name ("@https://..."), and a mirror whose hostname starts with registry. (https://registry.mirror.example/registry.npmjs.org/other/-/...) gets the name registry.npmjs.org/other, because "registry." matched the mirror's hostname.

Fix

  • Adds Entry::get_package_name_from_default_registry_url: strip https:// or http://, require registry.npmjs.org/ or registry.yarnpkg.com/ as the prefix of what remains, take the path up to the first /-/, and return None for an empty name or anything else. The name_to_use block calls it in place of the arithmetic and otherwise falls back to the spec name, as off-registry tarballs always did. Net 14 lines in yarn.rs; nothing else in the migrator changes.
  • Why this is correct: every index is taken from one left-to-right parse of the URL, so the slice bounds are ordered by construction and nothing is left to panic on. For the two default registries the layout is exactly <host>/<name>/-/... (with <name> possibly @scope/name), so the names produced for real URLs are the ones the old code produced (the remote entry of the yarn-stuff fixture is such a URL and its snapshot is unchanged), plus - for the - package. Tarballs on any other host were already named after the spec; anchoring the host check on the prefix makes the registry.-prefixed mirror behave like every other mirror.
  • Verified:
    • test/cli/install/migration/yarn-lock-migration.test.ts, "package names are only read from the path of default registry tarball URLs": one bun pm migrate over nine URL dependencies (both panicking shapes, the - package, the empty segment, the mirror, and the scheme/host combinations whose names are still extracted), asserting the full packages object of the resulting bun.lock and that the loopback registry saw no requests. With main's yarn.rs it dies with the panic above; each row was also run on its own against main (the /-/-after-host rows and the - package panic, //-/ writes an empty name, the mirror writes registry.npmjs.org/other, the remaining rows already produced the asserted names). Passes with this change.
    • The rest of the file passes with the debug build, all 14 snapshots unchanged (yarn-cli-repo needs --timeout on a loaded machine, the pre-existing debug-build timing in test(install): fix yarn-lock-migration yarn-cli-repo case under debug+ASAN #35377); lockfile-only.test.ts and the yarn cases in nested-overrides.test.ts and migrate.test.ts pass.
  • Relation to the other open yarn migration PRs (checked with git merge-tree against both branches):
  • Left alone: the is_default_registry check in the resolution block decides how the URL is stored (and accepts https:// only); sharing one predicate between it and the naming helper would change one of the two behaviors, and install: fix default-trusted lifecycle scripts being blocked after yarn.lock migration #38795 is editing that block.

Background

  • yarn v1 lockfile entries are keyed by their specs (name@range, or name@https://... for a dependency declared as a URL) and carry a resolved tarball URL. The migrator creates one bun lockfile package per entry and has to pick its name without downloading anything: normally from the spec, and for tarballs on the default registries from the URL, so that a dependency declared under one name but pointing at another package's registry tarball is recorded under the package's real name.
  • Registry tarball URLs have the fixed shape <registry>/<name>/-/<basename>-<version>.tgz, where <name> is one segment or @scope/name. /-/ separates the name from the file, which is why the path before it is a usable name on a known registry, and why the package literally named - is the one real package whose URL has /-/ directly after the host.
  • bun.lock packages entries are ["<name>@<version or url>", ...]; the name in that position is what bun install later works with, so an empty or hostname-shaped name there surfaces as a broken install rather than a migration error.
Earlier revisions of this PR

The first revision was this same helper. A review suggested folding the fix into Entry::get_package_name_from_resolved_url (the alias name parser) so that alias entries with the same URL shape would stop being named after the host; the second revision did that. #38948 has since been opened to name aliases from their specs and delete that parser entirely, which is the better fix for the alias side, so this PR went back to the self-contained tarball helper, which #38948 does not touch.

@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 34912fe5-4d39-4205-97a7-822a4ed146d4

📥 Commits

Reviewing files that changed from the base of the PR and between 88a6398 and ef0d36c.

📒 Files selected for processing (2)
  • src/install/yarn.rs
  • test/cli/install/migration/yarn-lock-migration.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:05 AM PT - Aug 15th, 2026

@robobun, your commit ef0d36c has some failures in Build #98306 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38803

That installs a local version of the PR into your bun-38803 executable, so you can run:

bun-38803 --bun

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review at ef0d36c (single commit on current main; waiting on CI for this push).

Reproduced on the released canary (b7a0431) and on main with bun pm migrate -f over a yarn.lock containing "x@https://evil.example/-/registry.npmjs.org/x.tgz": panic: slice index starts at 42 but ends at 20, exit 134. Any /-/ directly after the registry host panics the same way (27..26), including the tarball of the real package named -.

The new case in test/cli/install/migration/yarn-lock-migration.test.ts dies with that panic on main's yarn.rs and passes with this branch; the rest of the file passes with all 14 snapshots unchanged.

Previous push (12002d9) was green on all 177 lanes that ran; the only red was two darwin 14 aarch64 test jobs that expired without an agent.

Merges cleanly with #38948; conflicts with #38795 in the one block both rewrite (see the PR description for the intended order).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the left-to-right parse in get_package_name_from_registry_tarball_url makes the slice bounds ordered by construction, so the panic class is gone, and the fallback to base_name preserves prior behavior for every non-default-registry URL. The two inline notes are a pre-existing sibling bug and a test-hygiene nit; neither blocks.

Extended reasoning...

Overview

This PR fixes a slice-index panic in migrate_yarn_lockfile (src/install/yarn.rs) triggered by yarn.lock tarball URLs where /-/ appears before (or immediately after) the registry hostname. The old code ran two independent index_of searches over resolved ("/-/" and "registry.") and sliced between them without checking order. The fix extracts a helper that strips the scheme, requires registry.npmjs.org/ or registry.yarnpkg.com/ as an anchored prefix, then takes the path up to the first /-/; anything else returns None and the caller falls back to the spec name. The name_to_use block is flattened into an if let chain but is otherwise behavior-preserving (git branch first, tarball extraction second, base_name fallback). A new test in yarn-lock-migration.test.ts covers six URL shapes: three that used to panic, one mirror that used to mis-extract, and two default-registry URLs (scoped and http) whose extraction is preserved.

Security risks

None introduced. yarn.lock is untrusted input to bun install, and this change moves a reachable panic (DoS) to a graceful fallback. The new helper only narrows what counts as a registry URL (anchored prefix vs. substring anywhere), matching the is_default_registry check already used in the resolution block below. No new allocation, no new external I/O.

Level of scrutiny

Medium. This is package-manager code, but the change is a ~14-line pure helper plus a control-flow flatten of one block. I traced the new helper: strip_prefix on scheme → strip_prefix on host → index_of(path, "/-/") → slice path[..idx]. Every index comes from the same left-to-right walk over path, so 0 <= idx <= path.len() and the slice cannot panic; empty result is rejected. I compared the old and new name_to_use block: the git-repo arm, the tarball guard (is_direct_url_dep || is_remote_tarball || .tgz), and the base_name fallback are all preserved; the only behavioral change is that non-anchored / empty-name URLs now fall through to base_name instead of panicking or extracting a hostname-containing string.

Other factors

The inline findings are (1) a pre-existing bug in the adjacent get_package_name_from_resolved_url — untouched by this PR, correctly flagged as deferrable — and (2) an undrained stdout: "pipe" in the new test, which cannot deadlock here since bun pm migrate writes nothing to stdout for six entries. Both are worth addressing but neither affects correctness of the fix. The test asserts exact package names via toEqual on the parsed lockfile (strong invariant), points the registry at a closed port so it fails closed rather than reaching the network, and the PR description confirms it fails on the released build and passes with the change. Existing snapshots in the file are unchanged.

Comment thread test/cli/install/migration/yarn-lock-migration.test.ts Outdated
Comment thread src/install/yarn.rs Outdated
dylan-conway pushed a commit that referenced this pull request Aug 15, 2026
…d package (#38806)

### Problem
- Migrating a yarn v1 lockfile whose `npm:` alias points at a scoped
package records the unscoped tail of the name. For
`"my-node-types@npm:@types/node@20.11.5"` resolved from
`https://registry.yarnpkg.com/@types/node/-/node-20.11.5.tgz#...`,
`bun.lock` gets `"my-node-types": ["node@20.11.5", ...]` instead of
`["@types/node@20.11.5", ...]`, and the post-migration manifest pass
asks the registry for `node`. The same migration runs inside `bun
install` when only a `yarn.lock` exists, which is the `GET
https://registry.npmjs.org/monaco-editor-treemended/-/monaco-editor-treemended-1.83.16.tgz
- 404` reported in #27781. Reproduces on the released canary (b7a0431)
and on main.
- Cause: `Entry::get_package_name_from_resolved_url`
(`src/install/yarn.rs:239`), which names alias entries after their
tarball URL, walks back from `/-/` and checks the last path segment for
a leading `@`. In a scoped tarball URL the last segment is the unscoped
name and the scope is the segment before it, so the check never fires
and only the last segment is returned. The Zig original had the same
bug; #27782 and #27889 fixed it there and were closed when the Zig
sources went away.

### Fix
- `get_package_name_from_resolved_url` now splits the path before `/-/`
into its last segment and the one before it, and returns both when the
earlier one starts with `@`, otherwise just the last one. It returns
`None` (callers then fall back to the spec name) when there is no name
segment, where the old code returned an off-by-one slice or an empty
name.
- Why this is correct: the registry tarball layout is
`<registry>/<name>/-/<basename>-<version>.tgz`, and `<name>` is one
segment for plain packages and exactly two (`@scope/name`) for scoped
ones, so the segment before the name starting with `@` is the only thing
that distinguishes the two shapes; nothing else in the URL encodes it.
Plain packages take the same path as before, so their output is
unchanged.
- All three callers benefit: the package name written to the lockfile
(`yarn.rs:941`), the `(name, version)` key used to give the alias and a
plain entry of the same package one package id (`yarn.rs:843`), and the
alias detection that registers an entry under its spec name only when it
differs from the real name (`yarn.rs:1603`), which previously fired for
every scoped package.
- Left alone on purpose: `Entry::parse_npm_alias` also mis-splits
`npm:@scope/name@range` at the scope's `@`, but its only consumers are
the first dependency pass, whose slices are overwritten by the second
pass at `yarn.rs:1695` onward, and a `version` field spelled `npm:...`,
which yarn v1 never writes. Nothing observable depends on it.
- Test: `test/cli/install/migration/yarn-lock-migration.test.ts`,
"yarn.lock with npm aliases of a scoped package keep the scope". One
migration covers an unscoped alias, a scoped alias and a transitive
alias of `@types/node@20.11.5` (sharing one yarn entry, as yarn writes
patterns that resolve to the same tarball) next to a plain
`@types/node@18.19.0`, asserts the full `packages` object of the
resulting `bun.lock`, and runs against a loopback registry that records
which manifests the migration asks for (`@types/node` and the consumer,
no `node`). Fails on the released build with the three alias entries
reading `node@20.11.5`, passes with this change.
- The rest of `yarn-lock-migration.test.ts` (scoped-package and
real-world fixtures, 14 snapshots), `migrate.test.ts` and
`lockfile-only.test.ts` pass unchanged with the debug build; the
`yarn-cli-repo` output is byte-identical to its snapshot. That case
still brushes its 5s timeout under debug+ASAN, which is pre-existing and
tracked by #35377.
- No overlap with the other open yarn migration PRs: #38795 and #38803
rework the `name_to_use` block for tarball entries and leave this
function as is (after #38795 it becomes the only source of alias package
names).

Fixes #27781

### Background
- A yarn v1 `npm:` alias entry is keyed by the alias spec
(`alias@npm:real-name@range`) and carries the real package's `version`,
`resolved` tarball URL and `integrity`. The migrator stores the package
under its real name and hangs it in the tree under the alias, so it
needs the real name, and it currently recovers it from the tarball URL.
- `bun.lock` `packages` entries are `["<real name>@<version>",
"<registry, empty for the default>", {meta}, "<integrity>"]` keyed by
the node_modules path. The first element is what `bun install` later
downloads, which is why a wrong name there turns into a 404 or a
different package.
- After migrating,
`fetch_necessary_package_metadata_after_yarn_or_pnpm_migration` fetches
the manifest of every migrated npm package by the name recorded in the
lockfile to fill in bin/os/cpu. The test points that pass at a loopback
server and uses the requested names as a second, external view of which
package each alias was resolved to.
Comment thread src/install/yarn.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the earlier stdout-drain nit is fixed in d62aed2, and the comment-cop flag on the helper's doc comment was addressed in ba3de38 (it's now a one-liner). The pre-existing get_package_name_from_resolved_url scope bug is tracked separately in #38806 per the author's reply.

What was reviewed:

  • get_package_name_from_registry_tarball_url: prefix-strips scheme + exact registry host, slices to first /-/, rejects empty — no index arithmetic left to panic on; aligns with the is_default_registry check further down.
  • name_to_use refactor: control flow is equivalent (git → registry-tarball → base_name); for canonical npmjs/yarnpkg URLs (including scoped) the extracted name is unchanged.
  • New test: hermetic (closed-port registry), drains all pipes, asserts the full packages object; covers both panic shapes, the mirror mis-naming, and preserved scoped/plain extraction.
Extended reasoning...

Overview

Two files: a ~10-line helper plus a ~15-line rewrite of the name_to_use block in src/install/yarn.rs, and one new test in test/cli/install/migration/yarn-lock-migration.test.ts. The old code ran two independent index_of searches ("/-/" and "registry.") over a resolved tarball URL and sliced between the results, which panics whenever the registry-host substring appears after /-/ or when nothing sits between them. The new helper strips the scheme, requires registry.npmjs.org/ or registry.yarnpkg.com/ as the literal prefix of what remains, and returns the path up to the first /-/ (or None). The caller falls back to base_name on None, exactly as before.

Security risks

yarn.lock is untrusted input to bun install, so the pre-existing panic was a user-reachable abort. The fix converts every previously-panicking or mis-parsing shape into the existing spec-name fallback; there is no new parsing of untrusted data beyond strip_prefix/index_of, and no allocation. The stricter host check (prefix instead of substring) also stops a registry.-prefixed mirror from producing a package name containing a hostname. Net: strictly reduces attack surface.

Level of scrutiny

Medium — it touches bun install migration, but the change is a mechanical replacement of ad-hoc index arithmetic with a chain of strip_prefix/? that cannot produce out-of-order bounds. I traced the happy path for https://registry.npmjs.org/@scope/name/-/name-1.0.0.tgz through both old and new code and both yield @scope/name; the is_default_registry check at the resolution site already used the same starts_with test, so the two now agree on what counts as a default-registry URL. The only behavior change for non-panicking inputs is the mirror case, which was producing a nonsensical name and now falls back like every other non-default host.

Other factors

Since my previous review the author pushed d62aed2 (adds stdout.text() to the Promise.all, resolving the pipe-drain nit) and ba3de38 (shortens the helper's doc comment to one line, resolving the comment-cop bot). The pre-existing get_package_name_from_resolved_url scope-detection bug I noted is unchanged here and, per the author, is being fixed in #38806; this PR's code path never routes through that function. The new test is hermetic (BUN_CONFIG_REGISTRY at a closed port), uses tempDir/await using, drains all three streams concurrently, and asserts the exact packages object rather than a substring — it covers both panic shapes (direct-URL and semver-spec-resolved), the mirror mis-naming, and the preserved extraction for scoped and http:// yarnpkg URLs. No CODEOWNERS cover src/install/. This run's bug-hunt found nothing new.

@robobun
robobun force-pushed the farm/7e42c7ee/yarn-migrate-registry-tarball-name branch from ba3de38 to 12002d9 Compare August 15, 2026 06:51
@robobun robobun changed the title install: fix yarn.lock migration panic on tarball URLs with a registry host after "/-/" install: fix yarn.lock migration panic on tarball URLs with "/-/" right after the host Aug 15, 2026
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main now that #38806 has landed and reworked the shape (12002d9, description updated):

  • Instead of adding a second URL-to-name helper next to get_package_name_from_resolved_url, the name_to_use block now calls that parser, gated on Entry::is_default_registry_url. The parser itself skips <scheme>://<host>/ first, so the /-/-after-host shape yields no name at the alias call sites too (previously registry.yarnpkg.com@1.0.0 in bun.lock) rather than only in the tarball block.
  • The test grew rows for the shapes the earlier version did not pin: the real package named - (https://registry.npmjs.org/-/-/--0.0.1.tgz, which also panicked), an empty segment before /-/ (previously written with an empty name), the remaining scheme/host combinations, and an alias entry checked both in bun.lock and via the manifest name requested from a loopback registry.
  • Snapshots are unchanged, including the yarn-cli-repo fixture, whose entries all go through the modified parser.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — targeted crash fix that replaces the panicky two-search slice with a left-to-right ?-chained parse; earlier review notes (stdout drain, comment length) are addressed.

What was reviewed

  • Traced get_package_name_from_resolved_url on all 11 test URL shapes plus scoped/unscoped/path-prefixed registries; scheme+host stripping preserves output for well-formed URLs and returns None (not a hostname) for the malformed ones.
  • Checked the three other callers of the modified helper (npm-alias name, base_name, alias-registration loop): behavior unchanged for real yarn resolved URLs, and the new None fallback is already handled at each site.
  • is_default_registry_url anchors on the prefix, so mirror hosts and …/-/registry.npmjs.org/… no longer reach the extractor.
  • New test is hermetic (local Bun.serve, tempDir, both pipes drained) and asserts the full packages object with toStrictEqual.
Extended reasoning...

Overview

Fixes a slice-index panic in migrate_yarn_lockfile when a yarn.lock resolved URL has /-/ before (or immediately after) the registry host. Changes: adds Entry::is_default_registry_url, rewrites get_package_name_from_resolved_url to strip <scheme>://<host>/ before locating /-/, and collapses the nested name_to_use block into a single let-chain gated on the new prefix check. Adds one ~90-line test with 11 entry variants asserting exact migrated package names.

Security risks

None. This is best-effort name extraction from an already-untrusted lockfile string; the change strictly narrows what counts as a default-registry URL (prefix match instead of substring) and eliminates a reachable panic. No new I/O, no path construction from the extracted name at this stage.

Level of scrutiny

Medium. The rewritten helper is called from three other sites (npm-alias id assignment, base_name, alias-registration loop), so I traced each: for canonical https://<host>/<name>/-/… and https://<host>/@scope/<name>/-/… URLs the result is byte-identical to the base; for degenerate URLs (/-/ right after the host, empty segment) the helper now returns None instead of the hostname, which every caller already handles via unwrap_or_else(get_name_from_spec) or if let Some. The name_to_use rewrite is behavior-equivalent to the old nested block for well-formed default-registry URLs and falls through to base_name otherwise — the old code's only additional behavior on those inputs was the panic.

Other factors

  • All three prior review threads are resolved: stdout is now drained in Promise.all, the doc comment was trimmed, and the pre-existing scoped-alias issue was folded in via #38806 (this PR now builds on the rewritten helper rather than adding a parallel one).
  • Test uses toStrictEqual on the parsed bun.lock packages object and on requestedManifests, so every entry's name and resolution kind is pinned exactly; it's hermetic (local registry on port 0, per-test cache dir).
  • is_default_registry_url accepts http:// (the old substring check did too, and the test covers it) while the separate is_default_registry local in the resolution block remains https-only; that asymmetry is pre-existing and only affects whether the URL is stored, not the name.
  • No CODEOWNERS entry for src/install/.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Heads up from #38795: it now computes the hash-stripped URL once per entry (resolved_url) and gates this name block on is_direct_url_dep only, so whichever of the two lands second has a one-block conflict here. The combined shape is your extraction, gated on is_direct_url_dep, reading the stripped URL. As your description already notes, the pinned row becomes a registry package (["pinned@1.0.0", "", {}, ...]) once #38795 is in. Landing this one first is simplest; #38795 gets rebased either way.

…ht after the host

migrate_yarn_lockfile named a tarball entry by slicing the resolved URL
between two independent substring searches ("registry." and "/-/") and
assumed the host came first. A URL such as
https://evil.example/-/registry.npmjs.org/x.tgz, or one with nothing between
the registry host and "/-/" (including the tarball of the real package named
"-"), put the start past the end and aborted bun install / bun pm migrate with
"panic: slice index starts at 42 but ends at 20".

Replace the arithmetic with Entry::get_package_name_from_default_registry_url,
which strips the scheme, requires one of the two default registry hosts and
takes the path up to "/-/". Any other URL, or an empty name, keeps the name
from the spec, as off-registry tarballs always did.
@robobun
robobun force-pushed the farm/7e42c7ee/yarn-migrate-registry-tarball-name branch from 12002d9 to ef0d36c Compare August 15, 2026 15:52
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Reshaped again in ef0d36c, back to a self-contained fix:

Comment thread src/install/yarn.rs
Comment on lines +162 to +172
/// `https://registry.npmjs.org/@scope/name/-/name-1.0.0.tgz` -> `@scope/name`
pub(crate) fn get_package_name_from_default_registry_url(url: &[u8]) -> Option<&[u8]> {
let host_and_path = url
.strip_prefix(b"https://")
.or_else(|| url.strip_prefix(b"http://"))?;
let path = host_and_path
.strip_prefix(b"registry.npmjs.org/")
.or_else(|| host_and_path.strip_prefix(b"registry.yarnpkg.com/"))?;
let name = &path[..strings::index_of(path, b"/-/")?];
(!name.is_empty()).then_some(name)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The PR description and the 06:53/09:52 timeline comments describe a rework at 12002d9 (modifying get_package_name_from_resolved_url to skip <scheme>://<host>/, adding Entry::is_default_registry_url, fixing the registry.yarnpkg.com@1.0.0 alias case, and adding pinned + alias test rows), but ef0d36c has none of that — it adds a separate get_package_name_from_default_registry_url helper called only from the name_to_use block, leaves get_package_name_from_resolved_url untouched, and the test has only the nine URL-dependency rows. This looks like the reworked implementation was dropped in the rebase/squash to ef0d36c; either restore it, or rescope the description so it doesn't claim to fix the alias sibling or to test it (CLAUDE.md #11).

Extended reasoning...

What the mismatch is

The PR description (updated for 12002d9 per the 01:39 status comment) and the author's 06:53 timeline comment both describe a specific reworked shape:

  1. "get_package_name_from_resolved_url now skips <scheme>://<host>/ before looking for /-/" — so the alias call sites also stop returning the host as a package name.
  2. "behind a new Entry::is_default_registry_url check" — a separate predicate gating the name_to_use block.
  3. The alias bug is fixed — an npm: alias whose tarball URL has /-/ right after the host "previously registry.yarnpkg.com@1.0.0".
  4. The test has "a semver entry resolved to such a tarball" (the pinned row) and "an alias with /-/ after the host", checked "through a loopback registry, the name the alias was given".

The 09:52 comment even refers to "the pinned row of the new test" when discussing the #38795 rebase.

None of this is in ef0d36c — the commit CodeRabbit and the robobun build comment both name as the current head.

Step-by-step check against ef0d36c

(1) get_package_name_from_resolved_url is untouched. The diff has exactly two hunks in src/install/yarn.rs: adding the new helper get_package_name_from_default_registry_url at lines 162-172, and rewiring the name_to_use block to call it. The existing parser at lines 255-266 still starts with:

let path = &url[..strings::index_of(url, b"/-/")?];
let (prefix, name) = strings::rsplit_once_char(path, b'/')?;

For https://registry.yarnpkg.com/-/foo-1.0.0.tgz: index_of finds /-/ at 27, so path = b"https://registry.yarnpkg.com"; rsplit_once_char on / gives prefix = b"https:/", name = b"registry.yarnpkg.com". The function returns Some(b"registry.yarnpkg.com"). The three alias call sites (the is_npm_alias branch of the package-id loop, the base_name computation, and the trailing alias-registration loop) all still call this function, so an npm alias with that resolved URL is still named after the host — the third bullet in the PR description's Problem section is not fixed.

(2) No is_default_registry_url exists. grep -n is_default_registry_url src/install/yarn.rs returns nothing. The host check is folded into the new get_package_name_from_default_registry_url helper instead — the earlier "second helper" shape the 06:53 comment says was replaced ("Instead of adding a second URL-to-name helper next to get_package_name_from_resolved_url, the name_to_use block now calls that parser").

(3) The description says the name_to_use block "names the entry with Entry::get_package_name_from_resolved_url, the same parser npm: aliases use". It doesn't — it calls the new separate helper, so nothing is shared with the alias call sites.

(4) The test has exactly nine rows, all direct URL dependencies in the tarballs object. There is no pinned (semver) row, no npm: alias row, and the only registry assertion is expect(registryRequests).toStrictEqual([]) — nothing about "the name the alias was given". The description's Verified section explicitly claims all of these exist and were checked individually against main.

Why this matters

This is not a case of an imprecise description. The 06:53 comment is the author's own record of having reworked the implementation at 12002d9, and the description was updated to match. The 09:52 comment (posted after ef0d36c was already building at 8:52 per the robobun status) references a test row that doesn't exist. The commit at HEAD has the earlier shape the 06:53 comment says was replaced. This is the signature of a rebase/squash that dropped the reworked hunks.

If merged as-is:

  • The panic fix works, but the alias sibling — a same-class site with the identical /-/-after-host defect the PR title names, which the description explicitly commits to fixing — stays broken and untested (REVIEW.md, "Fix the whole class in the same PR").
  • The PR record permanently claims to fix and test something it doesn't (CLAUDE.md Convert all stored paths in .bun to project-relative #11: "never overstate what you got done").
  • Work the author believes is in the PR is silently discarded.

How to fix

One of:

  • Restore the dropped rework: modify get_package_name_from_resolved_url to strip <scheme>://<host>/ before the /-/ search, add Entry::is_default_registry_url, wire the name_to_use block to the shared parser behind that gate, delete get_package_name_from_default_registry_url, and add the pinned semver row and the alias row (with the loopback-registry manifest-name assertion) to the test — as the 06:53 comment and the description already describe.
  • Rescope: keep the current diff, but drop the third Problem bullet, the "same parser npm: aliases use" and is_default_registry_url sentences from Fix, and the pinned/alias claims from Verified. Note in the description that the alias sibling is intentionally left for install: name yarn.lock npm: alias packages after the alias spec, not the tarball URL #38948 (which per the description deletes get_package_name_from_resolved_url entirely).

Comment thread src/install/yarn.rs
Comment on lines +170 to +171
let name = &path[..strings::index_of(path, b"/-/")?];
(!name.is_empty()).then_some(name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The scoped sibling of the dash-package row isn't handled: for https://registry.npmjs.org/@scope/-/-/--0.0.1.tgz, the host-stripped path is @scope/-/-/--0.0.1.tgz and index_of(path, b"/-/") matches at byte 6 (inside the name), so the extracted name is @scope instead of @scope/-. Fix: when path starts with @, skip past the first / before searching for /-/. Extreme edge case (npm permits @scope/- by the same rule that permits the unscoped - package this PR already tests), no crash — just noting it since the PR explicitly enumerates this input class.

Extended reasoning...

What the bug is

Entry::get_package_name_from_default_registry_url (src/install/yarn.rs:170) strips the scheme and host, then takes everything before the first /-/ in the remaining path as the package name:

let name = &path[..strings::index_of(path, b"/-/")?];

For a URL dependency on a scoped package literally named -, the tarball URL is https://registry.npmjs.org/@scope/-/-/--0.0.1.tgz. After stripping https://registry.npmjs.org/, path = "@scope/-/-/--0.0.1.tgz". The bytes /-/ first appear at index 6 — the slash between @scope and its bare-name segment -, followed by that -, followed by the actual separator's leading slash — so name = path[..6] = "@scope", a bare scope with no package part.

Step-by-step trace

path = "@scope/-/-/--0.0.1.tgz":

index 0 1 2 3 4 5 6 7 8 9 10 11…
byte @ s c o p e / - / - / --0.0.1.tgz
  • strings::index_of(path, b"/-/") scans left-to-right and matches bytes 6–8 (/, -, /).
  • name = &path[..6] = b"@scope".
  • The correct separator is at index 8, which would give &path[..8] = b"@scope/-".

The unscoped - package (the dash-package test row) works because its path is -/-/--0.0.1.tgz, where the first /-/ at index 1 correctly yields - — there is no scope segment for the search to land inside.

Why nothing else prevents it

The caller at yarn.rs:962 uses the returned name directly (break 'blk name) with no further validation, so @scope becomes the package's recorded name in bun.lock ("@scope@https://…"). get_package_name_from_resolved_url (the sibling parser used for aliases) has the identical limitation for this input — it also searches for the first /-/ before splitting on the last slash — so nothing downstream corrects it.

Why it's the same input class this PR enumerates

The PR description explicitly calls out "the package literally named -" as "the one real package whose URL starts with /-/", adds it as the dash-package test row, and hardens the parser against it. npm's naming rules permit @scope/- by exactly the same rule that permits unscoped - (a name may not start with . or _; a lone - is fine). Per REVIEW.md's "Cover the variant matrix, not just the repro" and "Fix the whole class in the same PR", the scoped variant is the direct sibling of a case the PR chose to enumerate.

Impact

No crash — the slice bounds are ordered, so the PR's primary goal (never panic) is preserved. The migrated bun.lock records the entry as @scope@<url> instead of @scope/-@<url>. For a RemoteTarball resolution the name is largely a display label (installation fetches the URL directly), so the practical fallout is a wrong name in the lockfile rather than a failed install. Reachable only via a URL dependency on such a package or a hand-crafted yarn.lock — the same adversarial-input surface the rest of this PR hardens.

How to fix

When path starts with @, skip past the first / (the scope/name boundary) before searching for /-/, then include the scope prefix in the returned slice:

let sep_search_start = if path.first() == Some(&b'@') {
    strings::index_of(path, b"/")? + 1
} else {
    0
};
let sep = sep_search_start + strings::index_of(&path[sep_search_start..], b"/-/")?;
let name = &path[..sep];
(!name.is_empty()).then_some(name)

and add a "scoped-dash-package": "https://registry.npmjs.org/@scope/-/-/--0.0.1.tgz" row alongside dash-package in the new test, expecting "@scope/-@…".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant