Skip to content

Fix two version-comparison bugs in cryptohopper upgrade#7

Merged
pimfeltkamp merged 1 commit intomainfrom
fix-version-comparison-bugs
Apr 28, 2026
Merged

Fix two version-comparison bugs in cryptohopper upgrade#7
pimfeltkamp merged 1 commit intomainfrom
fix-version-comparison-bugs

Conversation

@pimfeltkamp
Copy link
Copy Markdown
Contributor

Why

Iter-35 audit of src/upgrade/github-release.ts found two bugs that haven't bitten yet because the CLI is at 0.6.0-alpha.1 and tags are pushed in order — but they will the moment we cross alpha.10, ship a hotfix, or both.

Bug 1: alpha.10 < alpha.2

compareVersions falls through to String#localeCompare on the entire prerelease tail. That's alphabetic — so "alpha.10" sorts before "alpha.2" because the second character (1 vs 2) is smaller.

Reproduction (against the unpatched code):

compareVersions("cli-v0.6.0-alpha.10", "cli-v0.6.0-alpha.2") -> -1
                                                              ^^^ should be > 0

User-visible effect once we ship cli-v0.6.0-alpha.10: anyone on cli-v0.6.0-alpha.2 runs cryptohopper upgrade, the SDK fetches the new release, the comparison says it's older, and the tool prints "✓ cryptohopper is up to date" — silently refusing the upgrade.

Fix

Adds comparePrerelease that follows SemVer 2.0.0 §11:

  • Identifiers consisting of only digits are compared numerically.
  • Identifiers with letters/hyphens are compared lexically in ASCII order.
  • Numeric identifiers always have lower precedence than non-numeric.
  • A larger set of prerelease fields beats a smaller one (when prefixes match).

Bug 2: fetchLatestRelease trusts GitHub's created_at order

GitHub's /releases API returns releases sorted by creation date, descending. For sequential tag pushes that matches version order. But:

  • Hotfix scenario: we ship cli-v0.7.0-alpha.1, then a security hotfix on the older line as cli-v0.6.1-alpha.1. cliReleases[0] is now 0.6.1-alpha.1 because it was created last.
  • The post-fix compareVersions(cli-v0.7.0-alpha.1, cli-v0.6.1-alpha.1) > 0 says the user is ahead, so upgrade does nothing — even though there is a newer-numbered release sitting at index 1.
  • Worse case: a user on 0.6.0 could be told they're on the latest because cliReleases[0] happens to be 0.6.1-prerelease and the user is technically newer. They'd never see 0.7.0.

Fix

Sort the filtered cli-v* list by version (highest first) with the now-correct compareVersions, then return the first element. Robust against any created_at order.

Test plan

9-case matrix run with tsx:

PASS  alpha.10 > alpha.2 (numeric prerelease compare)
PASS  alpha.2 < alpha.10
PASS  equal
PASS  release > prerelease
PASS  prerelease < release
PASS  minor bump beats prerelease bump
PASS  alpha < beta lexically
PASS  longer prerelease > shorter (when prefix matches)
PASS  rc > beta
9 passed, 0 failed

npm run typecheck and npm run build both clean. The CLI doesn't currently have a vitest setup; would happily wire one up in a follow-up if you want these to live as unit tests.

Compatibility

No public API change. compareVersions and versionFromTag keep their signatures. The new comparePrerelease is module-private. The only behavior change is for the corner cases that were broken — every previously-correct case still returns the same sign.

Bug 1: alpha.10 < alpha.2
  compareVersions used String#localeCompare on the full prerelease
  string, so "alpha.10" sorts before "alpha.2" alphabetically. SemVer
  2.0.0 specifies dot-separated identifiers, with numeric ones compared
  numerically. Adds a comparePrerelease helper that splits on "." and
  applies the SemVer rules:
    - all-digit identifiers compared numerically
    - mixed/letter identifiers compared lexically
    - numeric identifiers always have lower precedence than non-numeric
    - longer-prefix-matching set wins

Bug 2: fetchLatestRelease relied on GitHub's created_at order
  GitHub returns /releases sorted by created_at desc, which usually
  matches version order — but a hot-fix tag pushed after a newer
  release inverts that. Now sorts the filtered cli-v* list by version
  (highest first) using the now-correct compareVersions, so the latest
  is always the highest version regardless of publication order.

Verified with a 9-case matrix covering: alpha.10 vs alpha.2 both
directions, equality, release vs prerelease both directions, minor
bumps, alpha vs beta vs rc, and longer-prefix prereleases. typecheck
and build are clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pimfeltkamp pimfeltkamp merged commit 1d976f8 into main Apr 28, 2026
1 check passed
pimfeltkamp added a commit that referenced this pull request Apr 28, 2026
The 0.6.0-alpha.2 entry only mentioned the auth-fix dep bump from #9.
PRs #7 (cryptohopper upgrade version-comparison) and #8 (OAuth
browser-flow state validation + token-exchange timeout) also landed
on main without their own CHANGELOG lines. Folding all four bug
fixes under the same release since none has shipped yet.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant