Conversation
Semver strings longer than 8 bytes are offsets into the buffer they were parsed from. Three minimum-release-age code paths formatted a version against the other side's buffer: - bun outdated's column width pass formatted the lockfile's current version against the manifest buffer when no update/latest candidate passed the filter, so the Update and Latest columns were sized for the wrong text and the row overflowed the table. - the --verbose "[minimum-release-age] ... selected ..." line formatted the dependency's range (lockfile) against the manifest buffer. - the "was published within minimum release age" error formatted the manifest's version against the lockfile buffer.
|
Warning Review limit reached
Next review available in: 9 seconds 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 |
|
Reproduced all three on the unfixed binary with the mock registry from
The three new tests in that file fail the same way on the unfixed binary and pass with the fix; the whole file passes with a debug build. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — each of the four .fmt(buf) sites now receives the buffer the value was actually parsed from, matching how the same values are already formatted a few lines away.
What was reviewed:
outdated_command.rs:476/490:current_versionisresolution.npm().versionfrom the lockfile; the print pass at :696/:727 and the Current column at :465 already usestring_buf(lockfile), so the width pass now agrees.PackageManagerEnqueue.rs:1085:find_resultcomes fromloaded_manifest.find_by_version(...), so the manifest'sstring_bufis correct; the.unwrap()is already established at :1053.PackageManagerEnqueue.rs:2501:versionis the dependency range from the lockfile — the same value is passed withthis.lockfile.buffers.string_bytesat :2467 for matching;result.version/neweston the adjacent lines still correctly usemanifest_buf.- Tests: three new cases, one per site, with fixtures whose prerelease tags exceed the 8-byte inline limit; hermetic against the existing mock registry.
Extended reasoning...
Overview
Four one-line buffer-argument fixes across two files: the bun outdated column-width pass (outdated_command.rs:476/490) and two minimum-release-age messages in PackageManagerEnqueue.rs (:1085, :2501). Each site was formatting a semver Version/Group against the wrong string buffer (manifest vs lockfile), which mangles prerelease/build tags longer than the 8-byte inline threshold. Three regression tests are added to test/cli/install/minimum-release-age.test.ts plus two mock-registry packages with long prerelease tags.
Security risks
None. The changes affect only display output (table column widths, a --verbose diagnostic, and an error message). No parsing, resolution, or install-path logic is touched; the values themselves are unchanged, only which backing buffer their tag text is sliced from when printed.
Level of scrutiny
Low. Each fix is a mechanical buffer-argument swap whose correctness is directly verifiable from the value's provenance a few lines above, and each fixed site has a neighbouring line that already formats the same value with the now-chosen buffer (:465/696/727 for current_version, :2467 for the dependency range, :1053 for find_result's manifest). There are no control-flow, ownership, or allocation changes.
Other factors
The PR description explains the inline-vs-offset semver string representation and why the wrong buffer produces garbage rather than a crash, and explicitly lists related same-pattern sites that are intentionally deferred to separate PRs with their own fixtures. The new tests follow the file's existing conventions (mock registry, tempDir, concurrent pipe drain, bunEnv spread), assert exact output, and the outdated-table test also asserts uniform row widths as a structural check independent of the snapshot. No CODEOWNERS apply to the touched paths, and there are no prior human review comments.
This branch has not been deployed
1 participant
Problem
bun outdated --minimum-release-age(or withinstall.minimumReleaseAgeconfigured) prints a misaligned table when the installed version has a prerelease tag longer than 8 bytes (1.0.0-snapshot.20240101) and nothing passes the filter: theUpdate/Latestheader cells are sized for1.0.0- *while the row prints1.0.0-snapshot.20240101 *, so the row overflows the table.src/runtime/cli/outdated_command.rs:476and:490formatscurrent_version, which comes from the lockfile, againstmanifest.string_buf. The print pass (:696,:727) and theCurrentcolumn (:465) use the lockfile buffer. Reading the tag out of the wrong buffer returns an empty slice once the offset is past the end of the manifest buffer (or unrelated bytes before that), so the width is computed from the wrong text.src/install/PackageManager/PackageManagerEnqueue.rs:2501: the--verboseline[minimum-release-age] pkg@<range> selected X instead of Yformats the dependency's range (lockfile) against the manifest buffer. Observed:nightly-package@>=1.0.0-ightly.20240101h <2.0.0.src/install/PackageManager/PackageManagerEnqueue.rs:1085:Version "pkg@<version>" was published within minimum release age of N secondsformats the manifest's version against the lockfile buffer. Observed:Version "nightly-package@1.0.0-htly.20240101.tg".Fix
string_buffor the manifest's version. No other logic changes.outdated_command.rs:465/696/727, andPackageManagerEnqueue.rs:2465-2467passes the lockfile buffer for the same range when matching), so the fixed sites now agree with their neighbours.test/cli/install/minimum-release-age.test.ts, newdescribe("prerelease tags longer than an inline semver string"), one test per site. All three fail on the unfixed binary (misaligned table,>=1.0.0-ightly.20240101h,1.0.0-htly.20240101.tg) and pass with the fix; the whole file passes (52 tests) with the debug build.bun outdatedtest installs three up-to-date packages next tosnapshot-packageso that the prerelease tag's lockfile offset lies past the end ofsnapshot-package's manifest buffer; with a nearly empty lockfile the wrong buffer yields wrong bytes of the right length and the table lines up by accident. The test asserts the exact table.BUN_MANIFEST_CACHE=1, which makes bun treat the cached manifest as stale (the state any manifest is in five minutes after it was fetched) and take the exact-pin-from-cache path that prints the message.Background
Version, prerelease/build tags,Groupranges) do not own their text. Strings of up to 8 bytes are stored inline; longer ones are an offset and length into the string buffer they were parsed from, and every.fmt(buf)/.slice(buf)call has to be given that buffer.buffers.string_bytes(everything read frombun.lock/package.json: installed versions, dependency ranges) and each package manifest'sstring_buf(everything read from the registry: candidate versions).sliceis bounds-checked, so a wrong buffer produces garbage or an empty string rather than a crash, which is why this only shows up as misaligned or mangled output.minimum-release-ageis what makes thebun outdatedfallback branches reachable: without it,latestalways resolves andupdateonly fails when the installed version was unpublished.Related sites found while grepping for the same pattern, intentionally not in this PR
These have different triggers and need their own fixtures, so they are being tracked separately rather than bundled here:
src/semver/Version.rs:845:DiffFormatterprints its own build tag withother_bufwhen colours are on and the other version has no build tag (affectsbun outdatedwith build metadata in the registry version).src/runtime/cli/pm_view_command.rs:213:bun pm view/bun infopasses the manifest buffer as the buffer for a range parsed from the CLI argument.PackageManagerResolution.rs:152-202,Tag::clone_into) records tag offsets relative to a sub-slice oftags_bufand later slices against the whole buffer.PackageManagerEnqueue.rs:1085message: the exact-pin-from-cached-manifest path does not check that the cached manifest actually has publish times, so an abbreviated manifest cached by an earlier install lets an exact pin bypass--minimum-release-age. Handed off separately as its own bug.