semver: collapse a || union containing a match-all branch to * before the prerelease rule - #33739
semver: collapse a || union containing a match-all branch to * before the prerelease rule#33739robobun wants to merge 12 commits into
Conversation
… the prerelease rule node-semver's Range constructor collapses the whole range to * when any ||-alternative is the ANY comparator (see range.js L57-67). Under the default prerelease rule * rejects prereleases, so "x || 1.2.3-alpha.1" rejects 1.2.3-alpha.1 in npm but accepted it in Bun, which evaluated each branch independently. Track during parse whether any explicit ||-delimited branch is entirely match-all (*, x, X, >=0.0.0, ^*, ~*, hyphen-to-wildcard, or empty), set Flags::MATCH_ALL_BRANCH, and have Group::satisfies reject prereleases when the flag is set. Release versions are unaffected.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds match-all semver range detection, tracks ChangesSemver ANY-branch prerelease handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:31 AM PT - Jul 8th, 2026
❌ @robobun, your commit 978d5a7 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33739That installs a local version of the PR into your bun-33739 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…exact() A ||-branch consisting only of a skipped dist-tag token (the "1.0.0 || boop" shape the parser already handles via the catch-all arm) emitted no comparator and left branch_has_non_any false, so it was misclassified as the ANY comparator and set MATCH_ALL_BRANCH. node-semver loose mode drops such branches via .filter(c => c.length) before the collapse step, so "1.2.3-alpha.1 || latest" must keep union semantics. Set branch_has_non_any in the catch-all arm. Also add the MATCH_ALL_BRANCH guard to is_exact() to keep it in sync with get_exact_version().
|
CI on 978d5a7 is green on 74/75 lanes. The one red lane is
|
A ||-branch consisting solely of "^", "~" or ">=" with no operand emits a comparator synthesized from a zero-length Version::parse whose range passes is_match_all(), so it was misclassified as ANY and set MATCH_ALL_BRANCH. node-semver loose mode drops such branches via .filter(c => c.length) before the collapse step, so "1.2.3-alpha.1 || ^" must keep union semantics. Gate the match-all classification on parse_result.len > 0.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/semver/SemverRange.rs`:
- Around line 134-146: Document the caveat on SemverRange::is_match_all so the
!has_left() shortcut is clearly tied to valid ANY inputs only. Explain that
Range::default() is also produced by dangling bare operators like ^ or ~, and
that SemverQuery’s callers currently guard this with parse_result.len == 0. Add
the note directly to is_match_all (and, if needed, the
Range::default/Token::to_range path it relies on) so future callers understand
they must not use this method alone to distinguish "*" from a dangling operator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6959f36-2637-49aa-8759-021dab75bfec
📒 Files selected for processing (3)
src/semver/SemverQuery.rssrc/semver/SemverRange.rstest/cli/install/semver.test.ts
…in Group::eql The TokenTag::None continue path (reached by a bare '-' in a ||-branch) emitted no comparator and left branch_has_non_any false, so it was misclassified as ANY. node-semver loose mode drops it via .filter(c => c.length) before the collapse step, so "1.2.3-alpha.1 || -" must keep union semantics. Mirror the catch-all arm fix from 0d85b61. Also compare MATCH_ALL_BRANCH in Group::eql so two Groups that differ only in the flag are not considered equal (their satisfies() results can now differ for prerelease versions).
…arse position The previous len==0 check only covered a bare operator at end of input. Version::parse consumes leading v/=/whitespace into len and treats '|' as a clean delimiter (valid stays true), so neither len==0 nor !valid distinguishes a real wildcard from junk like 'vv', '==', '^v' in a ||-branch. Instead check whether the byte at the parse start is a digit or wildcard char; node-semver loose mode drops anything else before the collapse step.
…check node-semver only splits on '||'; a lone '|' is garbage that loose mode drops before the collapse step. The b'|' arm consumed one-or-more pipes and ran the MATCH_ALL_BRANCH bookkeeping unconditionally, so '| 1.2.3-alpha.1' set the flag and rejected the prerelease. Gate the flag logic on having seen at least two consecutive pipes; a lone pipe now marks the branch as non-ANY like other garbage.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/semver/SemverQuery.rs:1100-1102— Same class as theboop/^/-/vvsiblings, but at the hyphenate arm: thepossibly_hyphenatelookahead fires regardless oftoken.tag, so^0.0.0 - x(also~/>/</<=/=/vprefixed) reaches this arm, emits{Gte 0.0.0}, passesis_match_all(), and setsMATCH_ALL_BRANCH—Bun.semver.satisfies("1.2.3-alpha.1", "^0.0.0 - x || 1.2.3-alpha.1")now returnsfalse; pre-PR Bun and npm-loose (whoseHYPHENRANGELOOSEprefix[v=\s]*rejects^/~/>/</<=, so these are not hyphen ranges at all) returntrue. Consider gating the hyphenate arm's ANY classification the same way the fallthrough arm at line 1131 already does. Trigger is pathological, so not blocking.Extended reasoning...
What the bug is
The hyphenate arm's new check is
if !range.is_match_all() { branch_has_non_any = true; }— unlike the fallthrough arm at line 1131 it has no additional guard. Bun's hyphenate detection (thepossibly_hyphenateblock at ~line 1001-1027) is purely positional: it fires whenever<something> - <digit|x|X|*>appears, regardless oftoken.tag, and theWildcard::Majorhyphen arm always emits{Gte first_version}(discarding the operator prefix). So^0.0.0 - x,~0.0.0 - x,>0.0.0 - x,<0.0.0 - x,<=0.0.0 - x,=0.0.0 - x,v0.0.0 - xall reach the hyphenate arm withrange = {Gte 0.0.0},is_match_all()returns true,branch_has_non_anystays false, andMATCH_ALL_BRANCHis set at the next||.npm-loose does not collapse any of these.
^/~/>/</<=are not inHYPHENRANGELOOSE's[v=\s]*prefix, so npm doesn't treat them as hyphen ranges at all — the branch becomes e.g.[>=0.0.0 <0.0.1-0](from^0.0.0) or[>0.0.0], never the ANY singleton.=/vare matched byHYPHENRANGELOOSE, buthyphenReplacere-emits the capturedfromstring verbatim as>=${from}→>==0.0.0/>=v0.0.0, which theGTE0regex^\s*>=\s*0\.0\.0\s*$fails to match, so it is not normalized to''and the branch is[>=0.0.0](a real comparator, not the ANY sentinel).Verified against
semver@7.7.4:semver.satisfies("1.2.3-alpha.1", "^0.0.0 - x || 1.2.3-alpha.1", {loose:true}) // true semver.satisfies("1.2.3-alpha.1", "=0.0.0 - x || 1.2.3-alpha.1", {loose:true}) // true semver.satisfies("1.2.3-alpha.1", ">0.0.0 - x || 1.2.3-alpha.1", {loose:true}) // true semver.validRange("^0.0.0 - x || 1.2.3-alpha.1", {loose:true}) // "<0.0.1-0||1.2.3-alpha.1" — NOT "*" semver.validRange("=0.0.0 - x || 1.2.3-alpha.1", {loose:true}) // ">=0.0.0||1.2.3-alpha.1" — NOT "*" // controls that DO collapse (Bun and npm agree, both false): semver.validRange("0.0.0 - x || 1.2.3-alpha.1", {loose:true}) // "*" semver.validRange(">=0.0.0 - x || 1.2.3-alpha.1", {loose:true}) // "*"
Step-by-step trace (
"^0.0.0 - x || 1.2.3-alpha.1")^arm:token.tag = Caret,i=1.Version::parse("0.0.0 - x || …")→0.0.0,wildcard=None,len=5;i=6.possibly_hyphenatesucceeds (- x) — it does not checktoken.tag— sohyphenate=true.second_parsed.wildcard == Major→range = {left: {Gte 0.0.0}, right: Unset}(the^prefix is discarded).- Line 1100:
range.is_match_all()→ true (Gte, is_zero, no pre, no right).branch_has_non_anystays false. ||handler:!branch_has_non_any→MATCH_ALL_BRANCHset.Group::satisfies(1.2.3-alpha.1): prerelease + flag → false.
Pre-PR Bun: no flag machinery;
satisfies_prematched the second List{Eql 1.2.3-alpha.1}→ true.Why existing safeguards don't prevent it
The
has_real_operandguard added in 6663913 is only in the fallthroughelsearm (line 1131); the hyphenate arm at line 1100 has no equivalent. Andhas_real_operandalone wouldn't help anyway — for^0.0.0,input[i]='0'after the^arm advances, sohas_real_operandwould be true. The distinguishing signal istoken.tag: the hyphenate arm ignores it entirely, but npm'sHYPHENRANGEregex effectively rejects any leading operator exceptv/=(and even those dodgeGTE0via the>==0.0.0quirk). This is a distinct code path from every previously-addressed sibling arm (_catch-all,TokenTag::Nonecontinue, fallthroughtoken.to_range,b'|'separator).Impact
Nit. Strict behavioral regression from pre-PR Bun and npm-loose introduced by this PR (all seven prefixed forms returned
trueon main), but the trigger — an operator-prefixed0.0.0hyphenated with a major wildcard in a||branch next to a prerelease — is pathological and essentially never appears in realpackage.jsonfiles. Same rarity class as the siblings the PR already addresses; filing so the hyphenate-arm sibling of the same class is on record.Fix
Gate the hyphenate arm's ANY classification on the operator prefix — the only per-npm ANY hyphen shapes are
0.0.0 - <wc>and>=0.0.0 - <wc>, so e.g.if !range.is_match_all() || !matches!(token.tag, TokenTag::Version | TokenTag::Gte) { branch_has_non_any = true; }(with the caveat that=/vstill slip through as false positives vs npm's>==0.0.0quirk; a stricter version would additionally require the byte at the pre-operator position to be a digit/wildcard). Add"^0.0.0 - x","~0.0.0 - x",">0.0.0 - x","<0.0.0 - x","=0.0.0 - x","v0.0.0 - x"to the bare-operator test loop.
… parse
Gate the hyphenate arm's match-all check on token.tag in {Version, Gte}
so operator-prefixed hyphen ranges like '^0.0.0 - x' (which npm does not
treat as hyphen ranges) are not classified as ANY. Also require
parse_result.valid in the fallthrough arm so wildcard-prefixed garbage
that Version::parse rejects ('xy', '*a', '^xy') is not classified as
ANY. Closes most of the remaining adversarial edge cases surfaced in
review; the handful that remain ('xx', 'x.', '=0.0.0 - x') depend on
bun's lenient tokenizer accepting shapes npm's regex chain rejects and
are left as documented gaps.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/semver/SemverQuery.rs (1)
937-940: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOnly consume the second
|insrc/semver/SemverQuery.rs:937-940.|||currently skips the extra pipe too, so1.0.0 ||| *can behave like a clean||branch instead of leaving the lone|as loose-mode garbage. Add coverage for odd pipe runs like|||/||||alongside the lone-|cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/semver/SemverQuery.rs` around lines 937 - 940, The pipe-skipping logic in SemverQuery parsing is too greedy: the current loop in the query parser consumes every consecutive `|`, so odd runs like `|||` get treated like a valid `||` operator. Update the parsing branch around the `is_double` check to consume only the second pipe for a logical-or token and leave any extra `|` characters to be handled as loose-mode garbage. Add tests in the SemverQuery parsing coverage for lone `|` and odd/even pipe runs such as `|||` and `||||` to lock in the intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/semver/SemverQuery.rs`:
- Around line 1107-1112: The hyphen-range match-all check in
SemverQuery::parse_hyphen_range is too permissive by treating TokenTag::Gte as
acceptable on the left side, which misclassifies operator-prefixed ranges like
“>=0.0.0 - x” as hyphen ranges. Update the branch that sets branch_has_non_any
so it only whitelists TokenTag::Version for the left token when
range.is_match_all() is true, and keep the existing hyphen-range grammar check
in the SemverQuery logic aligned with npm’s “partial - partial” behavior.
---
Outside diff comments:
In `@src/semver/SemverQuery.rs`:
- Around line 937-940: The pipe-skipping logic in SemverQuery parsing is too
greedy: the current loop in the query parser consumes every consecutive `|`, so
odd runs like `|||` get treated like a valid `||` operator. Update the parsing
branch around the `is_double` check to consume only the second pipe for a
logical-or token and leave any extra `|` characters to be handled as loose-mode
garbage. Add tests in the SemverQuery parsing coverage for lone `|` and odd/even
pipe runs such as `|||` and `||||` to lock in the intended behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bff9b6e-8dc6-4eee-9252-28e7fe731ab9
📒 Files selected for processing (1)
src/semver/SemverQuery.rs
Pins the cases where a 3+ pipe run already agrees with npm-loose (leading |||, trailing ||||, ||| followed by a comparator). The one shape that diverges (exactly three trailing pipes with nothing after) cannot be fixed without trading it for a regression in '||| *', since bun consumes the pipe run as one separator; it joins the documented gap set (xx, x., =0.0.0 - x).
1 participant
Problem
Bun.semver.satisfies()diverges from npm'ssemverwhen a||-union contains a match-all branch (*,x,X,>=0.0.0) next to a branch that allows a prerelease: npm collapses the whole range to*and rejects the prerelease, Bun evaluated each branch and accepted it.This is the range parser behind
bun install, so apackage.jsonthat resolves one way under npm resolves differently under Bun.Cause
node-semver's
Rangeconstructor has a post-parse simplification: if any||-alternative is the ANY comparator set (value === "", produced by*,x,>=0.0.0, etc.), the whole range becomes[["*"]](range.js#L57-L67).*then rejects every prerelease under the default prerelease rule, so the prerelease-allowing sibling branch is discarded before it is ever tested.Bun's
List::satisfies_preiterated each||-branch independently, so the sibling branch matched.Fix
Track during parse whether any explicit
||-delimited branch is entirely a match-all comparator. A newRange::is_match_all()predicate recognizes the internal shapes that*/x/X/x.x/>=0.0.0/^*/~*/0.0.0 - xparse to (unset, or>=0.0.0with no prerelease and no right side). When a||closes such a branch, or the final branch after a||is match-all, setFlags::MATCH_ALL_BRANCHon theGroup.Group::satisfiesthen returnsfalsefor any prerelease version when the flag is set, matching the npm collapse. Release versions are unaffected (the flag is only consulted on the prerelease path).get_exact_version()also returnsNonewhen the flag is set, since a range with a match-all branch is semantically*, not an exact version.The flag is keyed to explicit
||separators, so it does not interact with the separate space-separated-comparators question (#32993) and"* 1.2.3-alpha.1"(no||) is unchanged.Verification
New cases in
test/cli/install/semver.test.tscovering every match-all spelling in both||positions, the release-version control, non-match-all siblings (1.x,>0.0.0,>=1.0.0,>=0.0.0-0), and*alongside a narrower comparator in the same branch.Differential against
semver@7.7.4over 174 version x range pairs (wildcards, prereleases,||unions, controls): 7 mismatches before, 0 after.