From bd6a13186b6d594f841768687fe9faadd9cfa97e Mon Sep 17 00:00:00 2001 From: Maxim Brashenko <39818683+j2h4u@users.noreply.github.com> Date: Sat, 4 Apr 2026 16:10:21 +0500 Subject: [PATCH] fix(hooks): use semver comparison for update check instead of inequality (#1617) * fix(hooks): use semver comparison for update check instead of inequality gsd-check-update.js used `installed !== latest` to determine if an update is available. This incorrectly flags an update when the installed version is NEWER than npm (e.g., installing from git ahead of a release). Replace with proper semver comparison: update_available is true only when the npm version is strictly newer than the installed version. Co-Authored-By: Claude Opus 4.6 (1M context) * fix(hooks): use semver comparison for update check instead of inequality gsd-check-update.js used `installed !== latest` to determine if an update is available. This incorrectly flags an update when the installed version is NEWER than npm (e.g., installing from git ahead of a release). Fix: - Move isNewer() inside the spawned child process (was in parent scope, causing ReferenceError in production) - Strip pre-release suffixes before Number() to avoid NaN - Apply same semver comparison to stale hooks check (line 95) update_available is now true only when npm version is strictly newer. Co-Authored-By: Claude Opus 4.6 (1M context) * test: add semver comparison tests for gsd-check-update isNewer() 12 test cases covering: major/minor/patch comparison, equal versions, installed-ahead-of-npm scenario, pre-release suffix stripping, null/empty handling, two-segment versions. Co-Authored-By: Claude Opus 4.6 (1M context) * docs: explain why isNewer is duplicated in test file Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- hooks/gsd-check-update.js | 16 ++++++- tests/semver-compare.test.cjs | 81 +++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 tests/semver-compare.test.cjs diff --git a/hooks/gsd-check-update.js b/hooks/gsd-check-update.js index 90e11ac5..323ce303 100755 --- a/hooks/gsd-check-update.js +++ b/hooks/gsd-check-update.js @@ -50,6 +50,18 @@ const child = spawn(process.execPath, ['-e', ` const path = require('path'); const { execSync } = require('child_process'); + // Compare semver: true if a > b (a is strictly newer than b) + // Strips pre-release suffixes (e.g. '3-beta.1' → '3') to avoid NaN from Number() + function isNewer(a, b) { + const pa = (a || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); + const pb = (b || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); + for (let i = 0; i < 3; i++) { + if (pa[i] > pb[i]) return true; + if (pa[i] < pb[i]) return false; + } + return false; + } + const cacheFile = ${JSON.stringify(cacheFile)}; const projectVersionFile = ${JSON.stringify(projectVersionFile)}; const globalVersionFile = ${JSON.stringify(globalVersionFile)}; @@ -81,7 +93,7 @@ const child = spawn(process.execPath, ['-e', ` const versionMatch = content.match(/\\/\\/ gsd-hook-version:\\s*(.+)/); if (versionMatch) { const hookVersion = versionMatch[1].trim(); - if (hookVersion !== installed && !hookVersion.includes('{{')) { + if (isNewer(installed, hookVersion) && !hookVersion.includes('{{')) { staleHooks.push({ file: hookFile, hookVersion, installedVersion: installed }); } } else { @@ -100,7 +112,7 @@ const child = spawn(process.execPath, ['-e', ` } catch (e) {} const result = { - update_available: latest && installed !== latest, + update_available: latest && isNewer(latest, installed), installed, latest: latest || 'unknown', checked: Math.floor(Date.now() / 1000), diff --git a/tests/semver-compare.test.cjs b/tests/semver-compare.test.cjs new file mode 100644 index 00000000..da8806c4 --- /dev/null +++ b/tests/semver-compare.test.cjs @@ -0,0 +1,81 @@ +/** + * Tests for the isNewer() semver comparison function used in gsd-check-update.js. + * + * WHY DUPLICATED: isNewer() lives inside a template literal string passed to + * spawn(process.execPath, ['-e', `...`]) — it runs in a detached child process + * that has no access to the parent module scope. This means it cannot be + * require()'d or imported from a shared module. The function is intentionally + * inlined in the spawn string so it works in the child process context. + * + * We mirror the implementation here so the logic is testable. If the hook's + * implementation diverges from this copy, the fix is to update this mirror — + * not to restructure the hook (which would require changing the spawn pattern + * across the entire hook architecture). + */ +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); + +// Mirror of isNewer() from hooks/gsd-check-update.js (inside spawn template) +function isNewer(a, b) { + const pa = (a || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); + const pb = (b || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); + for (let i = 0; i < 3; i++) { + if (pa[i] > pb[i]) return true; + if (pa[i] < pb[i]) return false; + } + return false; +} + +describe('isNewer (semver comparison)', () => { + test('newer major version', () => { + assert.strictEqual(isNewer('2.0.0', '1.0.0'), true); + }); + + test('newer minor version', () => { + assert.strictEqual(isNewer('1.1.0', '1.0.0'), true); + }); + + test('newer patch version', () => { + assert.strictEqual(isNewer('1.0.1', '1.0.0'), true); + }); + + test('equal versions', () => { + assert.strictEqual(isNewer('1.0.0', '1.0.0'), false); + }); + + test('older version returns false', () => { + assert.strictEqual(isNewer('1.0.0', '2.0.0'), false); + }); + + test('installed ahead of npm (git install scenario)', () => { + assert.strictEqual(isNewer('1.30.0', '1.31.0'), false); + }); + + test('npm ahead of installed (real update available)', () => { + assert.strictEqual(isNewer('1.31.0', '1.30.0'), true); + }); + + test('pre-release suffix stripped', () => { + assert.strictEqual(isNewer('1.0.1-beta.1', '1.0.0'), true); + }); + + test('pre-release on both sides', () => { + assert.strictEqual(isNewer('2.0.0-rc.1', '1.9.0-beta.2'), true); + }); + + test('null/undefined handled', () => { + assert.strictEqual(isNewer(null, '1.0.0'), false); + assert.strictEqual(isNewer('1.0.0', null), true); + assert.strictEqual(isNewer(null, null), false); + }); + + test('empty string handled', () => { + assert.strictEqual(isNewer('', '1.0.0'), false); + assert.strictEqual(isNewer('1.0.0', ''), true); + }); + + test('two-segment version (missing patch)', () => { + assert.strictEqual(isNewer('1.1', '1.0'), true); + assert.strictEqual(isNewer('1.0', '1.1'), false); + }); +});