ReviewOS
Reload

also looking at this

stacks/buddy

refactor: one update-type classifier, with the edge cases decided instead of accidental

#1439
Merged glennmichael123 wants to merge refactor/one-update-type into main
4 files +108 -116

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

src/buddy.tsmodified+3-48
@@ -1213,7 +1213,7 @@ export class Buddy {
12131213 }
12141214
12151215 // Determine update type
1216 const updateType = this.getUpdateType(constraintVersion, dep.version)
1216 const updateType = getUpdateType(constraintVersion, dep.version)
12171217
12181218 // Don't add prefix here - let the file updater handle prefix preservation
12191219 // This prevents double prefixes when the constraint already has one
@@ -1361,7 +1361,7 @@ export class Buddy {
13611361
13621362 if (latestVersion !== dep.currentVersion) {
13631363 // Determine update type
1364 const updateType = this.getUpdateType(dep.currentVersion, latestVersion)
1364 const updateType = getUpdateType(dep.currentVersion, latestVersion)
13651365
13661366 this.logger.info(`Update available: ${dep.name} ${dep.currentVersion} → ${latestVersion} (${updateType})`)
13671367
@@ -1532,7 +1532,7 @@ export class Buddy {
15321532
15331533 if (latestVersion !== dep.currentVersion) {
15341534 // Determine update type
1535 const updateType = this.getUpdateType(dep.currentVersion, latestVersion)
1535 const updateType = getUpdateType(dep.currentVersion, latestVersion)
15361536
15371537 this.logger.info(`Update available: ${dep.name} ${dep.currentVersion} → ${latestVersion} (${updateType})`)
15381538
@@ -1590,51 +1590,6 @@ export class Buddy {
15901590 return deduplicatedUpdates
15911591 }
15921592
1593 /**
1594 * Determine update type based on version comparison
1595 */
1596 private getUpdateType(current: string, latest: string): 'major' | 'minor' | 'patch' {
1597 try {
1598 // Clean version strings, including v, @ prefix for version ranges
1599 let cleanCurrent = current.replace(/^[v^~>=<@]+/, '')
1600 let cleanLatest = latest.replace(/^[v^~>=<@]+/, '')
1601
1602 // For GitHub Actions, normalize incomplete versions like "4" to "4.0.0"
1603 // This is important for proper semver comparison
1604 const normalizeVersion = (version: string): string => {
1605 const parts = version.split('.')
1606 while (parts.length < 3) {
1607 parts.push('0')
1608 }
1609 return parts.join('.')
1610 }
1611
1612 cleanCurrent = normalizeVersion(cleanCurrent)
1613 cleanLatest = normalizeVersion(cleanLatest)
1614
1615 if (Bun.semver.order(cleanLatest, cleanCurrent) <= 0)
1616 return 'patch'
1617
1618 // Use manual comparison for more accurate update type determination
1619 const currentParts = cleanCurrent.split('.').map(Number)
1620 const latestParts = cleanLatest.split('.').map(Number)
1621
1622 // Major version change
1623 if (latestParts[0] > currentParts[0])
1624 return 'major'
1625
1626 // Minor version change
1627 if (latestParts[0] === currentParts[0] && latestParts[1] > currentParts[1])
1628 return 'minor'
1629
1630 // Patch version change
1631 return 'patch'
1632 }
1633 catch {
1634 return 'patch'
1635 }
1636 }
1637
16381593 /**
16391594 * Check if latest version is strictly newer than current
16401595 */
src/utils/helpers.tsmodified+46-43
@@ -417,12 +417,21 @@ export function sortUpdatesByPriority(
417417}
418418
419419/**
420 * Parse version string and determine update type using Bun's semver
420 * Parse a version pair and classify the update, using Bun's semver.
421 *
422 * The one implementation of update-type classification. `Buddy` and
423 * `VersionResolver` used to carry private variants, and the three disagreed
424 * on exactly the inputs that matter: short actions tags ('v4'), v-prefixes,
425 * suffixed Docker tags and prereleases.
426 *
427 * Non-semver input ('latest', 'workspace:*', branch names) and downgrades
428 * deliberately degrade to 'patch' — the least-alarming label for a change
429 * that cannot be reasoned about.
421430 */
422431export function getUpdateType(currentVersion: string, newVersion: string): 'major' | 'minor' | 'patch' {
423432 // Remove any prefixes like ^, ~, >=, v, @, etc.
424 const cleanCurrent = currentVersion.replace(/^[v^~>=<@]+/, '')
425 const cleanNew = newVersion.replace(/^[v^~>=<@]+/, '')
433 const cleanCurrent = padNumericVersion(currentVersion.replace(/^[v^~>=<@]+/, ''))
434 const cleanNew = padNumericVersion(newVersion.replace(/^[v^~>=<@]+/, ''))
426435
427436 // If not an upgrade or equal, treat as patch (no-op or bugfix)
428437 try {
@@ -434,52 +443,46 @@ export function getUpdateType(currentVersion: string, newVersion: string): 'majo
434443 return 'patch'
435444 }
436445
437 try {
438 // Parse version parts manually for more reliable comparison
439 const currentParts = cleanCurrent.split('.').map(n => Number.parseInt(n, 10))
440 const newParts = cleanNew.split('.').map(n => Number.parseInt(n, 10))
446 // Parse parts manually: parseInt's truncation at the first non-digit is
447 // what lets a suffixed Docker tag ('3.19-alpine') classify by its numeric
448 // core rather than collapsing to 'patch'.
449 const currentParts = cleanCurrent.split('.').map(n => Number.parseInt(n, 10))
450 const newParts = cleanNew.split('.').map(n => Number.parseInt(n, 10))
441451
442 // Ensure we have at least 3 parts (major.minor.patch)
443 while (currentParts.length < 3) currentParts.push(0)
444 while (newParts.length < 3) newParts.push(0)
452 // Ensure we have at least 3 parts (major.minor.patch)
453 while (currentParts.length < 3) currentParts.push(0)
454 while (newParts.length < 3) newParts.push(0)
445455
446 const [currentMajor, currentMinor, currentPatch] = currentParts
447 const [newMajor, newMinor, newPatch] = newParts
448
449 // Major version change
450 if (newMajor > currentMajor) {
451 return 'major'
452 }
456 const [currentMajor, currentMinor] = currentParts
457 const [newMajor, newMinor] = newParts
453458
454 // Minor version change (same major, higher minor)
455 if (newMajor === currentMajor && newMinor > currentMinor) {
456 return 'minor'
457 }
459 if (newMajor > currentMajor)
460 return 'major'
458461
459 // Patch version change (same major and minor, higher patch)
460 if (newMajor === currentMajor && newMinor === currentMinor && newPatch > currentPatch) {
461 return 'patch'
462 }
463 }
464 catch {
465 // Fallback to semver satisfaction checks if parsing fails
466 try {
467 // Patch: within same minor series
468 if (Bun.semver.satisfies(cleanNew, `~${cleanCurrent}`))
469 return 'patch'
470 }
471 catch {}
462 if (newMajor === currentMajor && newMinor > currentMinor)
463 return 'minor'
472464
473 try {
474 // Minor: within same major series
475 if (Bun.semver.satisfies(cleanNew, `^${cleanCurrent}`))
476 return 'minor'
477 }
478 catch {}
479 }
465 // The guard above confirmed an upgrade with major and minor unchanged: a
466 // patch bump, or a prerelease moving under an unchanged triple
467 // (1.0.0-beta.1 → 1.0.0). This used to fall through to 'major', which let
468 // an excludeMajor filter block a beta graduation as a breaking change.
469 return 'patch'
470}
480471
481 // Otherwise major
482 return 'major'
472/**
473 * Pad a purely numeric short version to major.minor.patch.
474 *
475 * GitHub Actions tags are routinely 'v4', and Bun.semver.order('4.2.2', '4')
476 * is -1 — without padding, the upgrade guard misreads a real bump as a
477 * downgrade. Restricted to all-numeric parts so suffixed tags like
478 * '3.19-alpine' pass through untouched.
479 */
480function padNumericVersion(version: string): string {
481 const parts = version.split('.')
482 if (parts.length >= 3 || !parts.every(part => /^\d+$/.test(part)))
483 return version
484 while (parts.length < 3) parts.push('0')
485 return parts.join('.')
483486}
484487
485488/**
src/version/version-resolver.tsmodified+8-25
@@ -1,5 +1,6 @@
11import type { VersionRange } from '../types'
22import { semver } from 'bun'
3import { getUpdateType } from '../utils/helpers'
34
45export class VersionResolver {
56 /**
@@ -43,33 +44,15 @@ export class VersionResolver {
4344 }
4445
4546 /**
46 * Determine update type between two versions
47 * Determine update type between two versions.
48 *
49 * Delegates to the canonical classifier in `utils/helpers`. This class
50 * used to carry its own variant, which disagreed with the live scan path
51 * on exactly the awkward inputs: 'v2.0.0' read as no change (the v was
52 * never stripped) and 'latest' read as a fabricated major.
4753 */
4854 static getUpdateType(fromVersion: string, toVersion: string): 'major' | 'minor' | 'patch' {
49 // Strip leading semver range operators. Do NOT strip `@` — scoped npm package
50 // names start with `@` (e.g. @scope/pkg), so stripping it here would corrupt
51 // any caller that accidentally passed a full spec instead of a version.
52 const cleanFrom = fromVersion.replace(/^[\^~>=<]+/, '')
53 const cleanTo = toVersion.replace(/^[\^~>=<]+/, '')
54
55 const fromParts = cleanFrom.split('.').map((part) => {
56 const num = Number(part)
57 return Number.isNaN(num) ? 0 : num
58 })
59 const toParts = cleanTo.split('.').map((part) => {
60 const num = Number(part)
61 return Number.isNaN(num) ? 0 : num
62 })
63
64 // Ensure we have at least major.minor.patch structure
65 while (fromParts.length < 3) fromParts.push(0)
66 while (toParts.length < 3) toParts.push(0)
67
68 if (toParts[0] > fromParts[0])
69 return 'major'
70 if (toParts[0] === fromParts[0] && toParts[1] > fromParts[1])
71 return 'minor'
72 return 'patch'
55 return getUpdateType(fromVersion, toVersion)
7356 }
7457
7558 /**
test/version-classification.test.tsmodified+51-0
@@ -1,5 +1,6 @@
11import { describe, expect, it } from 'bun:test'
22import { getUpdateType } from '../src/utils/helpers'
3import { VersionResolver } from '../src/version/version-resolver'
34
45describe('Version Classification', () => {
56 describe('getUpdateType', () => {
@@ -52,3 +53,53 @@ describe('Version Classification', () => {
5253 })
5354 })
5455})
56
57/**
58 * Three parallel implementations of this classification existed — helpers,
59 * a private Buddy method, and VersionResolver — and disagreed on short
60 * actions tags, prereleases, suffixed Docker tags and non-semver input.
61 * These pin the decided semantics of the one that remains.
62 */
63describe('one classifier across the codebase', () => {
64 it('success case - a short actions tag reads as the bump it is', () => {
65 // Bun.semver.order('4.2.2', '4') is -1 — without numeric padding the
66 // upgrade guard misread this real upgrade as a downgrade.
67 expect(getUpdateType('v4', 'v4.2.2')).toBe('minor')
68 expect(getUpdateType('v4.2.2', 'v5')).toBe('major')
69 })
70
71 it('success case - prerelease movement under an unchanged triple is a patch', () => {
72 // These fell through to 'major', letting an excludeMajor filter block a
73 // beta graduation as if it were a breaking change.
74 expect(getUpdateType('1.0.0-beta.1', '1.0.0')).toBe('patch')
75 expect(getUpdateType('1.0.0-beta.1', '1.0.0-beta.2')).toBe('patch')
76 })
77
78 it('success case - a prerelease of the next major is still major', () => {
79 expect(getUpdateType('1.0.0', '2.0.0-rc.1')).toBe('major')
80 })
81
82 it('success case - suffixed docker tags classify by their numeric core', () => {
83 expect(getUpdateType('3.19-alpine', '3.20-alpine')).toBe('minor')
84 expect(getUpdateType('3.19-alpine', '4.0-alpine')).toBe('major')
85 })
86
87 it('edge case - non-semver input degrades to patch, never a fabricated major', () => {
88 expect(getUpdateType('latest', '1.0.0')).toBe('patch')
89 expect(getUpdateType('workspace:*', '1.0.0')).toBe('patch')
90 expect(getUpdateType('main', '1.0.0')).toBe('patch')
91 })
92
93 it('success case - VersionResolver answers identically', () => {
94 const cases: Array<[string, string]> = [
95 ['v4', 'v4.2.2'],
96 ['v1.0.0', 'v2.0.0'],
97 ['1.0.0-beta.1', '1.0.0'],
98 ['latest', '1.0.0'],
99 ['1.2.3', '1.3.0'],
100 ]
101
102 for (const [from, to] of cases)
103 expect(VersionResolver.getUpdateType(from, to)).toBe(getUpdateType(from, to))
104 })
105})