fix: subset false-positive with a prerelease eq and a differing bound - #889
fix: subset false-positive with a prerelease eq and a differing bound#889spokodev wants to merge 1 commit into
Conversation
`subset(sub, dom)` returned `true` when `sub` combined an exact prerelease
comparator (`=X.Y.Z-pre`) with a `>`/`>=`/`<`/`<=` bound of a different
`[major,minor,patch]` tuple, even though `sub` contains a version outside
`dom`:
subset('=1.1.2-alpha <3.1.0', '<1.0.0') // true, must be false
satisfies('1.1.2-alpha', '=1.1.2-alpha <3.1.0') // true (in sub)
satisfies('1.1.2-alpha', '<1.0.0') // false (not in dom)
In `simpleSubset`, the eqSet-vs-bound checks used
`satisfies(eq, String(gt), options)`, which rebuilds a full Range and
re-applies node-semver's prerelease-exclusion gating, so a prerelease `eq`
is judged not to satisfy a plain bound of another tuple. The code then
treats the eqSet as inconsistent and returns `null` (null set), which
`subset` reports as a subset of everything.
Test the eq version against the raw bound comparator instead
(`gt.test(eq)` / `lt.test(eq)`) — the same fix PR npm#867 applied to the
dom-side checks, which this left in place on the eqSet side.
|
Independent corroboration of this bug, plus a regression test that covers the class rather than specific inputs — offered in case it's useful for landing this. I hit this from the other direction, by differential fuzzing semver.subset('1.0.0-alpha.0 <2.0.0', '<0.0.1') // true
semver.satisfies('1.0.0-alpha.0', '1.0.0-alpha.0 <2.0.0') // true (in sub)
semver.satisfies('1.0.0-alpha.0', '<0.0.1') // false (not in dom)Why a table row may not be enough here: the existing suite is 100% green both with and without your fix. I ran So rather than more rows, this asserts the defining property over generated ranges:
Only that direction is checked — Against unpatched 7.8.5: With your fix applied: Two practical notes if you want it as-is: it has to live inside Patch: https://github.com/mrvonkalus/glasshouse/blob/main/patches/semver-subset-soundness-test.patch — happy to open it against your branch, or ignore this entirely if you'd rather keep the PR minimal. Either way the fix looks right to me. Disclosure: the fuzzing, analysis and this test were produced by an AI agent (Claude) working under my direction; harness and full results are at https://github.com/mrvonkalus/glasshouse and reproduce with |
What
subset(sub, dom)returnstrue(claimssub ⊆ dom) when it is actuallyfalse. It triggers whensubcombines an exact prerelease comparator (=X.Y.Z-pre) with a>/>=/</<=bound whose version has a different[major, minor, patch]tuple — a subset false-positive (the dangerous direction):Adding the
<3.1.0bound (which1.1.2-alphasatisfies) can only shrinksub, so it should never turn a non-subset into a subset — a monotonicity violation.Root cause
In
simpleSubset, the eqSet-vs-bound checks usedsatisfies(eq, String(gt), options).satisfiesbuilds a fullRangeand re-applies node-semver's prerelease-exclusion gating, so a prereleaseeq(1.1.2-alpha) is judged not to satisfy a plain bound of a different tuple (<3.1.0). The code then treats the eqSet as inconsistent withsuband returnsnull(a null set), whichsubsetreports as a subset of everything. In reality the eq version does satisfysub, because its own=eqcomparator admits its prerelease.This is the same "re-applying full-range prerelease gating on an isolated comparator" issue that PR #867 fixed on the dom side (
subset.js:177,:195), but it was left in place on the sub (eqSet) side (:127,:131).Fix
Test the eq version against the raw bound comparator (
gt.test(eq)/lt.test(eq)), mirroring PR #867.Tests
Added two
subsetcases (=1.1.2-alpha <3.1.0 ⊄ <1.0.0,<3.1.0-0 1.1.2-alpha ⊄ ~2.0). Both fail onmainand pass with the fix; issue #757's case (^10.2.0-beta.2 ⊂ ^10.2.0-beta.1) stays correct and the ranges tests pass.