From cc22321c4f0b0cb41612170bc379e3baced780d5 Mon Sep 17 00:00:00 2001 From: yyyyaaa Date: Thu, 12 Feb 2026 10:50:11 +0700 Subject: [PATCH] fix(pgpm): clear update cache after pgpm update and use semver comparison --- pgpm/cli/__tests__/update.test.ts | 222 ++++++++++++++++++++++++++++++ pgpm/cli/src/commands.ts | 12 +- pgpm/cli/src/commands/update.ts | 57 +++++++- 3 files changed, 288 insertions(+), 3 deletions(-) create mode 100644 pgpm/cli/__tests__/update.test.ts diff --git a/pgpm/cli/__tests__/update.test.ts b/pgpm/cli/__tests__/update.test.ts new file mode 100644 index 000000000..d5e8ef3cf --- /dev/null +++ b/pgpm/cli/__tests__/update.test.ts @@ -0,0 +1,222 @@ +jest.setTimeout(15000); +process.env.PGPM_SKIP_UPDATE_CHECK = 'true'; + +import * as fs from 'fs'; +import * as path from 'path'; +import semver from 'semver'; +import { appstash } from 'appstash'; + +import { clearUpdateCache, verifyInstalledVersion } from '../src/commands/update'; + +// ─── clearUpdateCache ──────────────────────────────────────────────── + +describe('clearUpdateCache', () => { + let cacheDir: string; + let cacheFile: string; + + beforeEach(() => { + const dirs = appstash('pgpm-test-update'); + cacheDir = dirs.cache; + cacheFile = path.join(cacheDir, 'update-check.json'); + + // Ensure the cache directory exists + fs.mkdirSync(cacheDir, { recursive: true }); + }); + + afterEach(() => { + // Clean up test cache directory + try { + if (fs.existsSync(cacheFile)) fs.unlinkSync(cacheFile); + if (fs.existsSync(cacheDir)) fs.rmdirSync(cacheDir); + // Clean up the parent .pgpm-test-update dir + const root = path.dirname(cacheDir); + if (fs.existsSync(root)) fs.rmSync(root, { recursive: true, force: true }); + } catch { + // best-effort cleanup + } + }); + + it('deletes existing cache file and returns true', () => { + fs.writeFileSync(cacheFile, JSON.stringify({ + latestVersion: '9.9.9', + timestamp: Date.now() + })); + expect(fs.existsSync(cacheFile)).toBe(true); + + const result = clearUpdateCache('pgpm-test-update'); + + expect(result).toBe(true); + expect(fs.existsSync(cacheFile)).toBe(false); + }); + + it('returns false when no cache file exists', () => { + // Ensure no cache file + if (fs.existsSync(cacheFile)) fs.unlinkSync(cacheFile); + + const result = clearUpdateCache('pgpm-test-update'); + expect(result).toBe(false); + }); + + it('returns false and does not throw for invalid tool name', () => { + // This should not throw even if appstash returns an unusual path + const result = clearUpdateCache('pgpm-test-update'); + expect(result).toBe(false); + }); +}); + +// ─── verifyInstalledVersion ────────────────────────────────────────── + +describe('verifyInstalledVersion', () => { + it('returns resolved version when pgpm is in PATH', async () => { + // This test validates the function shape; it may not find pgpm + // in PATH during CI, so we just validate the return contract + const result = await verifyInstalledVersion('pgpm', '99.99.99'); + expect(result).toHaveProperty('resolved'); + expect(result).toHaveProperty('matches'); + expect(typeof result.matches).toBe('boolean'); + }); + + it('matches is false when expected version does not match', async () => { + const result = await verifyInstalledVersion('pgpm', '0.0.0-impossible'); + // If pgpm is found, it won't match 0.0.0-impossible + // If pgpm is not found, resolved is null and matches is false + expect(result.matches).toBe(false); + }); + + it('returns null resolved when binary is not found', async () => { + // Use a binary name that definitely doesn't exist + const { verifyInstalledVersion: verify } = jest.requireActual('../src/commands/update') as typeof import('../src/commands/update'); + + // We can't easily test a missing binary for 'pgpm' specifically, + // but we verify the error handling path by checking the contract + const result = await verify('pgpm', '99.99.99'); + if (result.resolved === null) { + expect(result.matches).toBe(false); + } else { + expect(typeof result.resolved).toBe('string'); + } + }); +}); + +// ─── Semver comparison (the string > operator bug) ─────────────────── + +describe('semver comparison vs string comparison', () => { + it('string comparison fails for multi-digit versions', () => { + // This is the bug: JavaScript string "3.10.0" > "3.9.0" is FALSE + expect('3.10.0' > '3.9.0').toBe(false); + // But semver correctly identifies it as greater + expect(semver.gt('3.10.0', '3.9.0')).toBe(true); + }); + + it('string comparison fails for double-digit patch versions', () => { + expect('1.0.10' > '1.0.9').toBe(false); + expect(semver.gt('1.0.10', '1.0.9')).toBe(true); + }); + + it('semver correctly handles equal versions', () => { + expect(semver.gt('3.3.0', '3.3.0')).toBe(false); + }); + + it('semver correctly handles older versions', () => { + expect(semver.gt('3.2.0', '3.3.0')).toBe(false); + }); + + it('semver correctly handles newer major versions', () => { + expect(semver.gt('4.0.0', '3.99.99')).toBe(true); + }); +}); + +// ─── Update check integration (simulated cache scenario) ──────────── + +describe('update check cache lifecycle', () => { + let cacheDir: string; + let cacheFile: string; + const toolName = 'pgpm-test-lifecycle'; + + beforeEach(() => { + const dirs = appstash(toolName); + cacheDir = dirs.cache; + cacheFile = path.join(cacheDir, 'update-check.json'); + fs.mkdirSync(cacheDir, { recursive: true }); + }); + + afterEach(() => { + try { + const root = path.dirname(cacheDir); + if (fs.existsSync(root)) fs.rmSync(root, { recursive: true, force: true }); + } catch { + // best-effort cleanup + } + }); + + it('simulates the full bug scenario and fix', () => { + // Step 1: checkForUpdates would cache this during `pgpm deploy` + const cachedData = { + latestVersion: '3.4.0', + timestamp: Date.now() + }; + fs.writeFileSync(cacheFile, JSON.stringify(cachedData)); + + // Step 2: Verify cache exists (simulates the stale state) + expect(fs.existsSync(cacheFile)).toBe(true); + const cached = JSON.parse(fs.readFileSync(cacheFile, 'utf-8')); + expect(cached.latestVersion).toBe('3.4.0'); + + // Step 3: The fix — clearUpdateCache removes it + const cleared = clearUpdateCache(toolName); + expect(cleared).toBe(true); + expect(fs.existsSync(cacheFile)).toBe(false); + + // Step 4: Next checkForUpdates call would fetch fresh from registry + // instead of using the stale cached value + }); + + it('cache within TTL would be reused (demonstrating the bug)', () => { + const currentVersion = '3.3.0'; + const latestVersion = '3.4.0'; + const TTL = 24 * 60 * 60 * 1000; + + // Simulate a cache that was written 1 hour ago + const cachedData = { + latestVersion, + timestamp: Date.now() - (1 * 60 * 60 * 1000) // 1 hour ago + }; + fs.writeFileSync(cacheFile, JSON.stringify(cachedData)); + + // Read cache and check TTL (same logic as checkForUpdates) + const cached = JSON.parse(fs.readFileSync(cacheFile, 'utf-8')); + const withinTTL = Date.now() - cached.timestamp < TTL; + expect(withinTTL).toBe(true); + + // The old string-based check (the bug): + const stringHasUpdate = cached.latestVersion !== currentVersion + && cached.latestVersion > currentVersion; + // This happens to work for 3.4.0 vs 3.3.0, but... + expect(stringHasUpdate).toBe(true); + + // The fix — use semver: + const semverHasUpdate = semver.gt(cached.latestVersion, currentVersion); + expect(semverHasUpdate).toBe(true); + + // Now demonstrate where string comparison FAILS: + const stringWrongResult = '3.10.0' > '3.9.0'; // false! + const semverCorrectResult = semver.gt('3.10.0', '3.9.0'); // true + expect(stringWrongResult).toBe(false); + expect(semverCorrectResult).toBe(true); + }); + + it('expired cache is not reused', () => { + const TTL = 24 * 60 * 60 * 1000; + + // Simulate a cache that was written 25 hours ago + const cachedData = { + latestVersion: '3.4.0', + timestamp: Date.now() - (25 * 60 * 60 * 1000) + }; + fs.writeFileSync(cacheFile, JSON.stringify(cachedData)); + + const cached = JSON.parse(fs.readFileSync(cacheFile, 'utf-8')); + const withinTTL = Date.now() - cached.timestamp < TTL; + expect(withinTTL).toBe(false); + }); +}); diff --git a/pgpm/cli/src/commands.ts b/pgpm/cli/src/commands.ts index fbf360368..0c3b13d36 100644 --- a/pgpm/cli/src/commands.ts +++ b/pgpm/cli/src/commands.ts @@ -1,6 +1,7 @@ import { checkForUpdates } from '@inquirerer/utils'; import { CLIOptions, Inquirerer, ParsedArgs, cliExitWithError, extractFirst, getPackageJson } from 'inquirerer'; import { teardownPgPools } from 'pg-cache'; +import semver from 'semver'; import add from './commands/add'; import adminUsers from './commands/admin-users'; @@ -116,8 +117,15 @@ export const commands = async (argv: Partial, prompter: Inquirerer, pkgVersion: pkg.version, toolName: 'pgpm', }); - if (updateResult.hasUpdate && updateResult.message) { - console.warn(updateResult.message); + // Use proper semver comparison instead of the string-based check + // from checkForUpdates, which fails for multi-digit versions + // (e.g., "3.10.0" > "3.9.0" is false in string comparison) + const hasUpdate = updateResult.latestVersion + && semver.valid(updateResult.latestVersion) + && semver.valid(pkg.version) + && semver.gt(updateResult.latestVersion, pkg.version); + if (hasUpdate) { + console.warn(`Update available: ${pkg.version} -> ${updateResult.latestVersion}`); console.warn('Run pgpm update to upgrade.'); } } catch { diff --git a/pgpm/cli/src/commands/update.ts b/pgpm/cli/src/commands/update.ts index 7b898d99c..103843656 100644 --- a/pgpm/cli/src/commands/update.ts +++ b/pgpm/cli/src/commands/update.ts @@ -1,8 +1,13 @@ import { Logger } from '@pgpmjs/logger'; import { CLIOptions, Inquirerer, cliExitWithError, getPackageJson } from 'inquirerer'; -import { spawn } from 'child_process'; +import { appstash } from 'appstash'; +import { spawn, execFile } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; +import { promisify } from 'util'; import { fetchLatestVersion } from '../utils/npm-version'; +const execFileAsync = promisify(execFile); const log = new Logger('update'); const updateUsageText = ` @@ -19,6 +24,43 @@ Options: --dry-run Print the npm command without executing it `; +/** + * Invalidate the update-check cache so subsequent commands + * don't re-prompt after a successful update. + */ +export const clearUpdateCache = (toolName: string = 'pgpm'): boolean => { + try { + const dirs = appstash(toolName); + const cacheFile = path.join(dirs.cache, 'update-check.json'); + if (fs.existsSync(cacheFile)) { + fs.unlinkSync(cacheFile); + return true; + } + } catch { + // Best-effort: if cache cleanup fails, don't block the update + } + return false; +}; + +/** + * Verify that the PATH-resolved pgpm binary is actually the updated version. + */ +export const verifyInstalledVersion = async ( + pkgName: string, + expectedVersion: string +): Promise<{ resolved: string | null; matches: boolean }> => { + try { + const { stdout } = await execFileAsync('pgpm', ['--version'], { + timeout: 5000, + windowsHide: true + }); + const resolved = stdout.trim(); + return { resolved, matches: resolved === expectedVersion }; + } catch { + return { resolved: null, matches: false }; + } +}; + const runNpmInstall = (pkgName: string, registry?: string): Promise => { return new Promise((resolve, reject) => { const args = ['install', '-g', pkgName]; @@ -65,9 +107,22 @@ export default async ( try { await runNpmInstall(pkgName, registry); + + // Invalidate the update-check cache so subsequent commands + // don't re-prompt with a stale "update available" message + clearUpdateCache(); + const latest = await fetchLatestVersion(pkgName); if (latest) { log.success(`Successfully updated ${pkgName} to version ${latest}.`); + + // Verify the PATH-resolved binary is actually the new version + const { resolved, matches } = await verifyInstalledVersion(pkgName, latest); + if (resolved && !matches) { + log.warn(`pgpm in your PATH is still at ${resolved}.`); + log.warn('The global npm install may have gone to a different location than your shell resolves.'); + log.warn('Check your PATH or use the same package manager you originally installed pgpm with.'); + } } else { log.success(`npm install completed for ${pkgName}.`); }