fix: don't let a null comparator set mask the minimum of another union branch - #896
fix: don't let a null comparator set mask the minimum of another union branch#896maximilliangrand wants to merge 1 commit into
Conversation
…n branch
`minVersion` derives a candidate for each comparator set while ignoring
`<` / `<=` comparators, so a set that is a null set (eg `^1 ^2`) still
produces one. The candidate was only validated against the range once,
after the global minimum had already been chosen, so a null set whose
candidate is lower than every satisfiable branch's would win the
comparison, fail the final check, and make the whole call return null:
semver.satisfies('3.0.0', '^1 ^2 || >=3') // true
semver.minVersion('^1 ^2 || >=3') // null, expected 3.0.0
Validate each candidate before it becomes the running minimum instead.
Ranges that genuinely match nothing still return null, since every
candidate then fails the check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Some additional evidence on how often this fires, since a single hand-written repro makes it look like a corner case. I property-tested the range algebra against published semver 7.8.5, generating 300,000 random range pairs from a grammar over
Result on 7.8.5: So Note the null-set branch arises several different ways — mutually exclusive carets ( Re-running the identical harness against this branch: 19,229 → 0, with no new violations introduced in the other three properties. The existing suite is unaffected: Happy to add the harness under |
Minimal reproduction
Default options, no prereleases, one line:
minVersion(range)is documented as "Return the lowest version that can match", sonullmeans"nothing can possibly match this range". Here a version clearly does match.
Same shape with any null-set first branch:
The rule: whenever a union contains a comparator set that is a null set (
^1 ^2,>=2 <1,1.x 2.x) and that null set's computed lower bound is lower than the lower bound of everysatisfiable branch,
minVersionreturnsnullfor a range that has matching versions.Reproduces on 7.8.5, 7.3.8 and 6.3.1, so it has been present since
minVersionwas introduced in #241.Root cause
ranges/min-version.js:52-61. The loop derives a per-set candidatesetMinand keeps the globalminimum across all sets, then validates once, at the end:
setMinis computed while ignoring</<=comparators ("Ignore maximum versions", line 45), soan unsatisfiable set still yields a candidate:
^1 ^2expands to>=1.0.0 <2.0.0-0 >=2.0.0 <3.0.0-0and produces2.0.0. That candidate is lower than the3.0.0from the satisfiable
>=3branch, so it wins thegt()comparison, fails the singlerange.test()at line 57, and the function returns
null-- the usable branch's candidate was already discarded.Worth noting: the existing suite already covers null sets inside a union, but only in the direction
where the null set's candidate is higher and therefore loses the
gt()race:This is the same situation with the inequality reversed, which is why it slipped through.
This is distinct from the earlier
minVersionnull bugs #330 / #340 (fixed by #341), which wereabout ordering within a single comparator set. Those remain fixed --
minVersion('6 >=6.2.0 || 8 || >=9.3.0')returns6.2.0andminVersion('^2.16.2 ^2.16')returns2.16.2both before and after this change.The fix
Validate each candidate against the range before it becomes the running minimum, instead of once
after the minimum has been picked:
The result is exactly the minimum over valid candidates, independent of set order: the
gtguardonly ever skips candidates that are already >= the running minimum, and those can never become the
new minimum. Short-circuit ordering keeps this to at most one
range.test()per comparator set, andonly when the candidate would actually lower the running minimum.
Genuinely impossible ranges still return
null, because every candidate then failsrange.test(
'>4 <3','^1 ^2 || >4 <3'-- both covered by tests).Tests
Five tuples added to the existing table in
test/ranges/min-version.js. On the unmodifiedmainsource, assertions 43-46 fail and 47-48 pass:
With the fix all 48 pass.
['^1 ^2 || >4 <3', null]passes both ways -- it is a characterizationtest guarding the no-false-positive direction, not a fail-then-pass case.
Full suite: 51/51 test files pass, 0 failures, on both the untouched baseline and the patched tree,
so there are no pre-existing failures to separate out. Coverage stays at 100% for
ranges/min-version.jsand all files.npm run lint(eslint +template-oss-check) is clean.How it surfaced
A property/differential harness rather than manual inspection.
satisfies()is the spec-defining,exhaustively tested core, so I used it as an oracle and brute-forced it over a fixed 512-version
universe (major/minor/patch in 0..3 x 8 prerelease tags). A deterministic LCG generated random
ranges (x-ranges,
^,~, hyphen, bare/=/</<=/>/>=, 1-3 AND terms, 1-2 OR branches) andchecked the derived range algebra against that oracle -- among other properties:
minVersion(r)must satisfy
r, must be <= every version that matchesr, and must be non-null whenever anyversion matches.
8 seeds x 4000 iterations = 32,000 generated range pairs per target, 175,865 property-check
invocations, ~4 min per target, run against published
semver@7.8.5and against the patched tree:84 distinct counterexamples for this defect on baseline, 0 with the fix, and every other failure
class byte-identical.
The one delta --
minVersion-too-high354 -> 355 -- is not a regression. Case-level diffingshows exactly one case moving buckets,
'* >3.2.0 >=1.3.2 || 1.* - 0'underincludePrerelease:baseline returned
null(no version at all), the fix returns3.2.1(a genuinely matchingversion). It is still one notch high because of the separate, already-reported
>-bound prereleaseissue in #890, which behaves identically before and after this change
(
minVersion('>3.2.0', {includePrerelease: true})is3.2.1on both). So this case went fromcompletely wrong to correct-modulo-a-different-known-bug.
What I did not verify
the fix over that space, not universally.
minVersionbefore/after was noise-dominated in my measurements (medians 425ms vs302ms over 7 runs of 100k calls, with overlapping min/max), so I claim no measurable performance
change in either direction -- not a speedup.
of them are already covered by open PRs (fix: subset false-positive with a prerelease eq and a differing bound #889, fix: minVersion returns the true minimum for
>in includePrerelease mode #890);simplify-mismatchlooks like a real andseparate contract issue that I have not filed.