Repository navigation
test: the floor of the ICU version takes a version with more parts than it has - #47323
Merged
Merged
Conversation
process.versions.icu on macOS is what the system's ICU reports, and u_versionToString can print four parts. Bun.semver.order throws on a fourth. No machine is known to report one: macOS 26.7.1 reports 78.1.
Collaborator
|
Updated 12:05 AM PT - Oct 29th, 2026
@Jarred-Sumner, your commit b60fa72 is building: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
ICU version comparison test/js/node/process/process.test.js |
The test compares the first two components of process.versions.icu with the minimum version. A comment explains the component-count rule and semver handling of missing components. |
Possibly related PRs
- oven-sh/bun#47314: Raises the ICU minimum in the same test and upgrades bundled ICU to 79.1.
- oven-sh/bun#47281: Raises the ICU minimum in the same test and upgrades bundled ICU to 79.1.
Suggested reviewers: dylan-conway
Priority:
Merge Risk: 
b60fa
-
The test accommodates longer ICU version strings without changing production behavior. No actionable merge-blocking risk is identified.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the ICU version floor change and the handling of versions with more components. The wording is somewhat awkward, but it is specific and related to the main change. Description check The description explains the problem, the implementation, and the verification results. It does not use the template headings, but it covers both required topics and is sufficiently complete. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Bun.semver.order reads a missing part as the highest, so 72.1 is 72.1.MAX and 72.1.0 is below it. With three parts kept, 72.1.0.1 against a floor of 72.1 still failed.
Jarred-Sumner
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to a review comment on #47281.
ICU version does not regresshandsprocess.versions.icutoBun.semver.order. On macOS the version is what the system's ICU reports, andu_versionToStringcan print three or four parts. The test beside it already allows for that (/^\d+\.\d+(\.\d+)*$/). Two things go wrong then:Bun.semver.orderthrows on a fourth part.72.1is72.1.MAX, and72.1.0is below it.The floors have two parts, so the comparison now takes the first two parts of the version.
72.178.1(macOS 26.7.1)74.2.0.1Invalid SemVer: 74.2.0.172.1.0.1Invalid SemVer: 72.1.0.172.1.071.9.9.9Invalid SemVer: 71.9.9.9No machine is known to report more than two parts, so nothing fails today.