diff --git a/.changeset/verify-installed-version-after-upgrade.md b/.changeset/verify-installed-version-after-upgrade.md new file mode 100644 index 00000000000..90fe3f2ad8e --- /dev/null +++ b/.changeset/verify-installed-version-after-upgrade.md @@ -0,0 +1,6 @@ +--- +'@shopify/cli-kit': patch +'@shopify/cli': patch +--- + +Verify the installed version after `shopify upgrade` and fail instead of reporting a false success when the upgrade didn't complete diff --git a/packages/cli-kit/src/public/node/upgrade.test.ts b/packages/cli-kit/src/public/node/upgrade.test.ts index a84f4bac8af..cb0177d0670 100644 --- a/packages/cli-kit/src/public/node/upgrade.test.ts +++ b/packages/cli-kit/src/public/node/upgrade.test.ts @@ -1,4 +1,4 @@ -import {isDevelopment} from './context/local.js' +import {isDevelopment, isUnitTest} from './context/local.js' import {currentProcessIsGlobal, inferPackageManagerForGlobalCLI} from './is-global.js' import {checkForCachedNewVersion, packageManagerFromUserAgent, PackageManager} from './node-package-manager.js' import {exec, isCI} from './system.js' @@ -10,8 +10,10 @@ import { versionToAutoUpgrade, } from './upgrade.js' import {Notification, fetchNotifications} from './notifications-system.js' -import {isPreReleaseVersion} from './version.js' +import {globalCLIVersion, isPreReleaseVersion} from './version.js' +import {mockAndCaptureOutput} from './testing/output.js' import {getAutoUpgradeEnabled} from '../../private/node/conf-store.js' +import {CLI_KIT_VERSION} from '../common/version.js' import {vi, describe, test, expect, beforeEach} from 'vitest' vi.mock('./notifications-system.js', async (importOriginal) => { @@ -31,6 +33,7 @@ vi.mock('./version.js', async (importOriginal) => { return { ...actual, isPreReleaseVersion: vi.fn(() => false), + globalCLIVersion: vi.fn(), } }) @@ -146,6 +149,12 @@ describe('runCLIUpgrade', () => { beforeEach(() => { // Mock isDevelopment to return false by default (not in CLI development mode) vi.mocked(isDevelopment).mockReturnValue(false) + // context/local.js is auto-mocked; restore isUnitTest so rendered banners are collected + // by mockAndCaptureOutput instead of printed to the real console. + vi.mocked(isUnitTest).mockReturnValue(true) + // By default the post-install verification finds the current version installed, + // which counts as success when no newer version was cached. + vi.mocked(globalCLIVersion).mockResolvedValue(CLI_KIT_VERSION) }) test('runs the install command via exec for a global npm install', async () => { @@ -219,6 +228,63 @@ describe('runCLIUpgrade', () => { // Then expect(exec).not.toHaveBeenCalled() }) + + test('reports the verified installed version on success', async () => { + // Given + vi.mocked(currentProcessIsGlobal).mockReturnValue(true) + vi.mocked(inferPackageManagerForGlobalCLI).mockReturnValue('npm') + vi.mocked(exec).mockResolvedValue() + vi.mocked(checkForCachedNewVersion).mockReturnValue('4.7.1') + vi.mocked(globalCLIVersion).mockResolvedValue('4.7.1') + const outputMock = mockAndCaptureOutput() + outputMock.clear() + + // When + await runCLIUpgrade() + + // Then + expect(outputMock.info()).toContain("You're now on version 4.7.1") + }) + + test('throws when the install lands an older version than expected (e.g. a stale private registry)', async () => { + // Given + vi.mocked(currentProcessIsGlobal).mockReturnValue(true) + vi.mocked(inferPackageManagerForGlobalCLI).mockReturnValue('npm') + vi.mocked(exec).mockResolvedValue() + vi.mocked(checkForCachedNewVersion).mockReturnValue('4.7.1') + vi.mocked(globalCLIVersion).mockResolvedValue('3.94.3') + const outputMock = mockAndCaptureOutput() + outputMock.clear() + + // When/Then + await expect(runCLIUpgrade()).rejects.toThrow( + 'Expected to be on version 4.7.1, but version 3.94.3 is now installed', + ) + expect(outputMock.info()).not.toContain('Shopify CLI upgraded') + }) + + test('throws when the install leaves the CLI on the current version instead of the expected one', async () => { + // Given + vi.mocked(currentProcessIsGlobal).mockReturnValue(true) + vi.mocked(inferPackageManagerForGlobalCLI).mockReturnValue('npm') + vi.mocked(exec).mockResolvedValue() + vi.mocked(checkForCachedNewVersion).mockReturnValue('4.7.1') + vi.mocked(globalCLIVersion).mockResolvedValue(CLI_KIT_VERSION) + + // When/Then + await expect(runCLIUpgrade()).rejects.toThrow(`Expected to be on version 4.7.1, but version ${CLI_KIT_VERSION}`) + }) + + test('throws when the installed version cannot be verified', async () => { + // Given + vi.mocked(currentProcessIsGlobal).mockReturnValue(true) + vi.mocked(inferPackageManagerForGlobalCLI).mockReturnValue('npm') + vi.mocked(exec).mockResolvedValue() + vi.mocked(globalCLIVersion).mockResolvedValue(undefined) + + // When/Then + await expect(runCLIUpgrade()).rejects.toThrow("Couldn't verify the Shopify CLI version after upgrading") + }) }) describe('versionToAutoUpgrade', () => { diff --git a/packages/cli-kit/src/public/node/upgrade.ts b/packages/cli-kit/src/public/node/upgrade.ts index ba9eb541cf5..b1fcf2055c0 100644 --- a/packages/cli-kit/src/public/node/upgrade.ts +++ b/packages/cli-kit/src/public/node/upgrade.ts @@ -15,9 +15,11 @@ import {outputContent, outputDebug, outputInfo, outputToken, outputWarn} from '. import {renderSuccess} from './ui.js' import {cwd, moduleDirectory, sniffForPath} from './path.js' import {exec, isCI} from './system.js' -import {isPreReleaseVersion} from './version.js' +import {globalCLIVersion, isPreReleaseVersion} from './version.js' +import {AbortError} from './error.js' import {getAutoUpgradeEnabled, setAutoUpgradeEnabled, runAtMinimumInterval} from '../../private/node/conf-store.js' import {CLI_KIT_VERSION} from '../common/version.js' +import {lt as semverLt} from 'semver' export {getAutoUpgradeEnabled, setAutoUpgradeEnabled} @@ -104,9 +106,29 @@ export async function runCLIUpgrade(options: RunCLIUpgradeOptions = {}): Promise Now upgrading by running: ${outputToken.genericShellCommand(installCommand)}...`, ) await exec(command, args, {stdio: 'inherit'}) + + // A zero exit code doesn't guarantee the right version landed: the version check above + // queries the public npm registry, while the install goes through whatever registry the + // user has configured. A private registry with a stale `latest` tag (or one that serves + // stale metadata on auth errors) can "successfully" install an outdated version. Verify + // what's actually installed before claiming success. + const installedVersion = await globalCLIVersion() + if (!installedVersion) { + throw new AbortError( + "Couldn't verify the Shopify CLI version after upgrading.", + outputContent`Check the installed version by running ${outputToken.genericShellCommand('shopify version')}.`, + ) + } + const expectedVersion = newerVersion ?? CLI_KIT_VERSION + if (semverLt(installedVersion, expectedVersion)) { + throw new AbortError( + `Failed to upgrade Shopify CLI. Expected to be on version ${expectedVersion}, but version ${installedVersion} is now installed.`, + 'Your package manager may be resolving @shopify/cli from a registry with outdated versions. Check your npm registry configuration and try again.', + ) + } renderSuccess({ headline: 'Shopify CLI upgraded.', - body: newerVersion ? `You're now on version ${newerVersion}.` : "You're now on the latest version.", + body: `You're now on version ${installedVersion}.`, }) } else if (projectDir) { await upgradeLocalShopify(projectDir, CLI_KIT_VERSION)