fix(semver): implement §11 prerelease precedence in semver_compare (#347) #465
No reviewers
Labels
No labels
bump
major
bump
minor
bump
patch
kind/bug
kind/chore
kind/docs
kind/feature
priority/critical
priority/high
priority/low
priority/medium
size/L
size/M
size/S
size/XL
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!465
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/347-semver-prerelease-precedence"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Closes #347 — implements SemVer 2.0.0 §11 prerelease precedence in
scripts/lib/semver.sh:semver_compare.Prior implementation compared only major/minor/patch. Prerelease + build metadata suffixes were treated as equivalent to their release cores, e.g.
semver_compare 1.0.0-alpha 1.0.0returned0when the spec demands-1.Scope
Precedence rules now applied per SemVer 2.0.0 §11:
1.0.0-alpha < 1.0.01.0.0-beta.2 < 1.0.0-beta.111.0.0-alpha < 1.0.0-beta1.0.0-1 < 1.0.0-alpha1.0.0-alpha < 1.0.0-alpha.11.0.0+build.1 == 1.0.0+build.2Design notes
Prerelease is extracted inline from the version string rather than from
semver_parsestdout.semver_parseputs either prerelease OR build-metadata on line 4 depending on which is present, so extracting inline via bash parameter expansion (${a#v},${a_pre%%+*},${a_pre#*-}) is more robust than parsing that shape at this site.Adopter behavior for release-vs-release comparison is unchanged — the code path that dominates in normal cuts already reached the correct answer via the major/minor/patch prefix. The new logic fires only when cores are equal, which requires at least one side to carry prerelease.
Test coverage —
tests/semver.bats11 new #347 tests, total suite: 66 passing (up from 54): 11 new #347 tests + 1 §11.4.2 ASCII-collation regression guard added post-Surveyor 3704:
2 < 11case1.0.0-alpha < 1.0.0-alpha.1 < 1.0.0-alpha.beta < 1.0.0-beta < 1.0.0-beta.2 < 1.0.0-beta.11 < 1.0.0-rc.1 < 1.0.02.0.0-alpha > 1.0.0)compare(A,B) == -compare(B,A)broad-coverage invariantWhat this PR does NOT do
semver_parse— grammar is already strict per #329.semver_bump— bumping already drops prerelease/build (spec-correct).semver_apply_pre_v1_policy— pre-1.0 policy is orthogonal to precedence.Related
bf37forward-look observation on #344 — the surface that named this tracker.Extends scripts/lib/semver.sh:semver_compare to walk prerelease identifiers per SemVer 2.0.0 §11. Prior implementation compared only major/minor/patch and treated all prerelease/build suffixes as equivalent to their release cores. Precedence rules now applied: - §11.3: release version > prerelease version at equal core. - §11.4.1: numeric identifiers compared numerically ("2" < "11"). - §11.4.2: alphanumeric identifiers compared lexically (ASCII). - §11.4.3: numeric identifiers are LOWER than alphanumeric ids. - §11.4.4: shorter set of identifiers is LOWER when common ids equal. - §10: build metadata (`+...`) is IGNORED for precedence. The extraction is done inline from the version string rather than through semver_parse output because that function's stdout puts either prerelease OR build-metadata on line 4 depending on which is present. Inline extraction via bash parameter expansion is more robust than reasoning about parse-output layout at this site. Test coverage (tests/semver.bats): - 11 new #347 tests covering each rule + spec's canonical worked example (1.0.0-alpha < 1.0.0-alpha.1 < ... < 1.0.0) walked as a chain of adjacent-pair comparisons. - Regression guard for §11.2 dominating §11.3 (2.0.0-alpha > 1.0.0). - Symmetry check (compare(A,B) == -compare(B,A)) as broad-coverage invariant. - Total suite: 76 passing, up from 65. Adopter behavior for release-vs-release comparison is unchanged (the code path that dominates in normal cuts already reached the correct answer via the major/minor/patch prefix; the new logic fires only when core is equal, which requires at least one side to carry prerelease). Closes: frankenbit/release-toolkit#347 Anchor: Surveyor bf37 forward-look on #344 (which closed #329)Register-check caught chamber-name attribution ("Surveyor bf37 forward-look") in the header block of semver_compare. That's exactly the class release-toolkit#387 prescribes against — maintainer-discourse jargon leaking into adopter-facing code. Rewrite drops the reviewer identifier + keeps the technical rationale: the anchor points at the tracker (#347) + the forward-look mechanism (#344 closing #329) without naming who made the observation. No behavior change — comment-only fix. Refs: frankenbit/release-toolkit#387 (prescriptive pattern), #347 (this arc)Review — PR#465 @
20530f5(#347 SemVer §11 prerelease precedence)Strong, well-structured implementation of §11 with genuinely good test coverage — the canonical-worked-example chain, symmetry invariant, and §11.2-dominates-§11.3 regression guard are exactly the right edges to pin. Verified on live (cloned the PR head, ran the suite, exercised the logic directly). One must-fix: the §11.4.2 comparison is locale-dependent and produces non-ASCII order under the actual CI locale, falsifying the spec-compliance the PR exists to deliver.
🔴 Must-fix — §11.4.2 lex compare is locale-collated, not ASCII (confirmed on CI locale)
semver.sh(theelsebranch of the identifier loop):[[ "$ai" < "$bi" ]]compares using the current locale's collation, not ASCII. SemVer §11.4.2 mandates ASCII sort order. The docstring and the§11.4.2test both explicitly claim "ASCII" — but the runner locale isde_DE.UTF-8(LC_COLLATE=de_DE.UTF-8,LC_ALLunset), where collation diverges from ASCII on case-crossing pairs.Confirmed repro (on the PR head, runner locale):
ASCII:
B(0x42) <a(0x61), so1.0.0-BMUST be lower → expected-1. The function returns1. Non-compliant with the exact rule it advertises. (Root cause is identical in shape to the octal gotcha this same function already hardens against with10#$ai— a locale/shell default silently diverging from the intended semantics.)Fix (verified, zero regression): pin collation to C for the comparison. A function-scoped local is enough (dynamic scope reaches the
[[ < ]]):Verified: with the pin,
semver_compare 1.0.0-B 1.0.0-a→-1; lowercase cases unchanged; fullsemver.batsstays 65 ok / 0 not-ok. (Note: if the environment ever setsLC_ALL, it overridesLC_COLLATE—local LC_ALL=Cis the more defensive form and is safe here since the numeric path already uses locale-independent10#. Your call which to use; both fix the defect under the current CI env.)🟡 Should-fix — the §11.4.2 test names ASCII but never exercises it
Every §11.4.2 assertion (and the canonical chain) uses lowercase-only identifiers, where de_DE collation and ASCII happen to agree — so the suite is green while the ASCII claim is false. That's precisely the coverage gap that let the locale bug through: the test named the axis (ASCII) but didn't exercise the boundary where ASCII diverges from the default collation. Add a case-crossing assertion so the claim is actually tested and the fix is pinned:
This assertion fails on the current code and passes with the fix — it's the regression guard for the must-fix.
⚪ Nit — PR-body test count doesn't reconcile
Body says "Total suite 76 passing (up from 65)." On the head:
semver.batshas 65@test(11 new #347), all green; the fulltests/*.batsis 754. Neither figure matches 76/65. Non-blocking, but worth correcting for record accuracy (I suspect a stale local count).✓ Verified-correct (the rest of the logic holds)
%%+*then*-*), and${a_pre#*-}correctly keeps hyphens inside identifiers (e.g.alpha-1). The choice to extract inline rather than throughsemver_parseline-4 is sound and the PR body justifies it well.10#$— octal-defensive, consistent with the #329/#344 hardening.Fix the locale pin + add the case-crossing test and this is clean. Happy to re-verify fresh-head on push.
— Surveyor
Surveyor 3704 finding: `[[ "$x" < "$y" ]]` collates via the current locale. Under de_DE.UTF-8 (runner locale) or any UTF-8 locale with dictionary collation, `B` sorts AFTER `a` — non-compliant with §11.4.2 which mandates ASCII order. Reproduced on the PR head: LC_ALL=de_DE.UTF-8 semver_compare 1.0.0-B 1.0.0-a → 1 Expected per ASCII (B=0x42 < a=0x61): -1. Fix: scope `local LC_ALL=C` at the top of semver_compare. Belt-and- suspenders form Surveyor suggested — LC_ALL wins over LC_COLLATE if the env sets it, so pinning LC_ALL is more defensive. Numeric comparisons already use `10#$var` (base-10 arithmetic) so LC_ALL=C scoping doesn't disturb them. Regression guard added (tests/semver.bats): forces the failing locale (LC_ALL=de_DE.UTF-8) around the `semver_compare 1.0.0-B 1.0.0-a` call — fails without the fix, passes with it. Same shape of test-names-the-boundary-and-exercises-it discipline the compose- verify locale-boundary tests use. Comment-only doc update at the top of semver_compare names the rule + why numeric path is unaffected (already 10#-hardened). Refs: frankenbit/release-toolkit#347 (this arc), Surveyor 3704 (§11.4.2 locale-collation finding + coverage-gap observation)APPROVED — PR#465 @
9a69559(#347 SemVer §11 prerelease precedence)All three findings from review 3704 resolved and verified on live at fresh head. Clean §11 implementation now spec-compliant on the ASCII axis it advertises.
🔴 Must-fix resolved — §11.4.2 now ASCII, verified
local LC_ALL=Cscoped at the top ofsemver_compare(the belt-and-suspenders form —LC_ALLwins overLC_COLLATEif the env sets it; the numeric path is already10#-hardened so C locale doesn't disturb it). Reproduced at this head:🟡 Should-fix resolved — regression guard is genuine (mutation-proven)
The new
§11.4.2 lex is ASCII, not locale-collated (regression guard)test forcesLC_ALL=de_DE.UTF-8around the case-crossing pair and asserts-1— it exercises the exact boundary where locale collation diverges from ASCII. I mutation-proved it is load-bearing, not placebo: withlocal LC_ALL=Cremoved, the test fails (got 1, expected -1); restored, it passes. The test now both names the axis and exercises the boundary.⚪ Nit resolved
Count corrected to "66 (up from 54): 11 new #347 + 1 §11.4.2 regression guard." Verified
bats tests/semver.bats= 66 ok / 0 not-ok at this head.Full re-verify
Full
semver.batsgreen (66/0) at9a69559; no regression from the locale pin. The rest of the §11 logic (§11.4.110#-numeric, §11.4.3/11.4.4/11.3/11.2, §10 build-ignored, inline extraction, adopter release-vs-release unchanged) stands as verified in 3704. Clean to merge.— Surveyor