Skip to content

fix(arcup): version_gt ignores prerelease tags in comparisons - #255

Closed
batuhankocyigit wants to merge 1 commit into
circlefin:mainfrom
batuhankocyigit:fix/arcup-prerelease-version-comparison
Closed

fix(arcup): version_gt ignores prerelease tags in comparisons#255
batuhankocyigit wants to merge 1 commit into
circlefin:mainfrom
batuhankocyigit:fix/arcup-prerelease-version-comparison

Conversation

@batuhankocyigit

Copy link
Copy Markdown

fix(arcup): version_gt ignores prerelease tags entirely, so it can't tell a release from its own release candidate

The bug

version_gt() in arcup/arcup strips everything after the first - before
comparing versions:

ver1="${ver1%%-*}"
ver2="${ver2%%-*}"

This means the prerelease suffix (-rc.1, -rc.2, ...) is discarded
before any comparison happens — two versions that only differ in their
prerelease tag are indistinguishable to this function, in both directions:

version_gt "0.3.0" "0.3.0-rc.1"        # → false (should be true: a stable
                                        #   release outranks its own rc)
version_gt "0.3.0-rc.2" "0.3.0-rc.1"   # → false (should be true: rc.2 is
                                        #   newer than rc.1)

The existing test suite already asserts the other direction is correct
(version_gt "0.3.0-rc.1" "0.3.0" → false, i.e. "same prerelease base is
not newer" — this still passes), which shows prerelease correctness was
already a design concern here; the reverse direction and rc-vs-rc just
weren't covered.

Where this bites: check_installer_up_to_date and update_arcup both
call version_gt to decide whether a newer arcup is available. If a user
is running a prerelease build of arcup and the true latest stable release
has the same major.minor.patch, they'll never be prompted to update,
since the function reports "not newer" in both directions.

The fix

Keep the numeric major.minor.patch comparison as-is (unchanged), but once
that's equal, apply semver prerelease precedence instead of discarding the
suffix:

  • A version with no prerelease tag outranks one with a prerelease tag
    (1.2.3 > 1.2.3-rc.1).
  • Two prerelease tags are compared identifier-by-identifier (split on .):
    numeric identifiers compare numerically (rc.10 > rc.9, not a string
    comparison), others compare lexically; a prerelease that's a prefix of
    the other (fewer identifiers) is lower.

No change to the plain major.minor.patch path — every previously-passing
comparison keeps the same result.

Testing

  • Added 4 regression cases to arcup/test_arcup.sh's existing
    test_version_comparison (same bash TAP-style harness already used in
    this file): stable-vs-own-rc, rc.2-vs-rc.1, rc.1-vs-rc.2, and
    rc.10-vs-rc.9 (numeric, not lexical, ordering).
  • Ran the full suite: bash arcup/test_arcup.sh28/28 passing (24
    pre-existing + 4 new), including the pre-existing "same prerelease base
    is not newer" case, confirming no regression.
  • bash -n arcup/arcup — syntax OK.
  • shellcheck arcup/arcup — no warnings.

Notes for reviewers

  • Scoped entirely to version_gt; no other function touched.
  • I did not change normalize_version's regex or accepted formats — only
    how two already-valid version strings are ordered.

@osr21 osr21 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is real and your analysis of it is accurate — but this ground has been claimed for three weeks. #212 (danilaverbena, opened July 20) fixes this exact bug — it's the PR attached to issue #205, which your description restates without citing. #212 has been through two review rounds and is strictly ahead of this implementation on the cases where the two differ. Details:

1. Build metadata: #212 handles it, this PR errors on it. SemVer allows +build suffixes, and version_gt here never strips them. Ran your branch directly:

version_gt "1.0.1+build.5" "1.0.0"   → returns "not newer" (wrong), with:
  arcup: line 286: [: 1+build.5: integer expression expected
version_gt "1.0.0+build-123" "1.0.0" → same class of failure

The +build survives into the patch field, the -gt integer test throws, and both comparisons fall through to "not newer". Worse, build metadata containing a hyphen (1.0.0-rc.1+build-123) gets the hyphen-split treatment and the metadata leaks into the prerelease identifier comparison. #212 hit this same class of issue in its first review round and fixed it by stripping +… up front (v="${v%%+*}") before the prerelease split — that ordering handles both cases correctly, and #212's test file asserts it (assert_le "1.0.0-rc.1+build-123" "1.0.0-rc.1" and friends).

2. Test placement: #212's composes better with the CI wiring in flight. This PR adds assertions inline to arcup/test_arcup.sh — the file that #223 (+235/-2) and #243 already modify, joining a three-way rebase cluster. #212 put its 25 assertions in a new arcup/version_gt_test.sh, which (a) touches nothing anyone else is editing, and (b) already matches the discovery glob in #247's make test-arcup, so it gets CI coverage automatically the moment that lands.

3. Small factual nit, same one I've flagged on two other PRs now: the suite on main has 25 assertions, not 24 ("28/28" should be 29 — your own branch prints 29 ok lines, which I confirmed by running it).

To be clear about what this PR gets right: the core precedence logic is sound for hyphen-only versions (verified 0.3.0 > 0.3.0-rc.1, rc-vs-rc numeric ordering works), the numeric-vs-lexical identifier split is correct semver, and the writeup is honest about scope. If #212 didn't exist this would be a solid start needing one hardening round. But #212 does exist, is further along, covers a superset of cases, and belongs to the contributor who filed the issue. The fair move is to close this in favor of #212 — and if you want this effort to count, reviewing #212's prerelease-identifier loop against your own implementation's edge cases (e.g. what each does with a purely-numeric first identifier like 1.0.0-1.2) would be genuinely additive: a second implementer is exactly who spots the cases both might have missed.

@batuhankocyigit

Copy link
Copy Markdown
Author

Thanks for the thorough review, @osr21 — verified all three points against my
branch and you're right on every count (build-metadata reproduces exactly as
you showed, and my suite does print 29 not 28). Closing this in favor of #212,
which already handles the cases mine misses and is further along in review.

@osr21

osr21 commented Aug 11, 2026

Copy link
Copy Markdown

Appreciate the fast, graceful close, @batuhankocyigit — verifying against your own branch before responding is exactly the right instinct.

To close the loop on the cross-review I suggested: I went ahead and ran the edge cases where your implementation and #212 could plausibly diverge, directly against #212's current branch. All correct per semver.org §11:

GT: 1.0.0-1.2 > 1.0.0-1.1        (numeric identifiers compared numerically)
GT: 1.0.0-1.2 > 1.0.0-1          (more identifiers wins when prefix equal)
LE: 1.0.0-1  <= 1.0.0-alpha      (numeric ranks below alphanumeric)
GT: 1.0.0-alpha.1 > 1.0.0-alpha
LE: 1.0.0-2  <= 1.0.0-10         (no lexical-compare trap: 2 < 10)
LE: 1.0.0-01 <= 1.0.0-1          (leading zero: 01 == 1 numerically, equal → not greater)
GT: 1.0.1+build.5 > 1.0.0        (the case that broke here — handled)
LE: 1.0.0-rc.1+build-123 <= 1.0.0-rc.1

Its own suite is also green (version_gt: 25 passed, 0 failed). So #212 checks out on the full identifier-comparison surface, not just the headline cases — one more independent data point for maintainers that it's safe to merge.

If you're looking for genuinely open ground in this repo: as far as I've mapped it, the arcup/CI cluster is fully claimed (#212, #223, #243, #247, #249, #254 all pending merge), so fresh issues from actually running the node/tooling — rather than another pass over arcup — are where a new contribution would land cleanly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants