From 3593fed806ec93840c763311319dd81074da6b46 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Thu, 21 May 2026 18:31:00 +0100 Subject: [PATCH 1/3] feat(update): detect Homebrew installs and upgrade via brew The update command previously fell back to `npm i -g` for any install it couldn't identify as pnpm. On a Homebrew-installed CLI that either creates a conflicting npm-global binary or fails with EACCES against the brew-owned node prefix. Detect a brew install (the running script resolves into a brew Cellar) and delegate to `brew upgrade ` instead, inheriting brew's own progress output. Adds an optional `brewFormula` option for brew-distributed CLIs to set. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 3 +- src/commands/update.test.ts | 55 ++++++++++++++++++++++ src/commands/update.ts | 92 +++++++++++++++++++++++++++++++++---- 3 files changed, 140 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index a619562..87f9935 100644 --- a/README.md +++ b/README.md @@ -114,11 +114,12 @@ registerUpdateCommand(program, { currentVersion: packageJson.version, configPath: getConfigPath('todoist-cli'), changelogCommandName: 'td changelog', + brewFormula: 'doist/tap/todoist-cli', withSpinner, }) ``` -`update` checks the configured channel's npm dist-tag (`stable` → `latest`, `pre-release` → `next`), compares against `currentVersion`, and shells out to `npm i -g` (or `pnpm add -g` if `npm_execpath` indicates pnpm). `update switch --stable | --pre-release` flips the persisted `update_channel` field via `updateConfig`, preserving any sibling keys. Both subcommands accept `--json` / `--ndjson`. Errors are `CliError` (`INVALID_FLAGS`, `UPDATE_CHECK_FAILED`, `UPDATE_INSTALL_FAILED`, or the canonical `CONFIG_*` codes if the config file is broken). +`update` checks the configured channel's npm dist-tag (`stable` → `latest`, `pre-release` → `next`), compares against `currentVersion`, and shells out to `npm i -g` (or `pnpm add -g` if `npm_execpath` indicates pnpm). When the CLI was installed via Homebrew (its binary resolves into a brew `Cellar`), it runs `brew upgrade ` instead — set `brewFormula` on brew-distributed CLIs (the brew formula may lag the npm publish, so an upgrade can be a no-op until the formula is bumped). `update switch --stable | --pre-release` flips the persisted `update_channel` field via `updateConfig`, preserving any sibling keys. Both subcommands accept `--json` / `--ndjson`. Errors are `CliError` (`INVALID_FLAGS`, `UPDATE_CHECK_FAILED`, `UPDATE_INSTALL_FAILED`, or the canonical `CONFIG_*` codes if the config file is broken). The semver helpers (`parseVersion`, `compareVersions`, `isNewer`, `getInstallTag`, `fetchLatestVersion`, `getConfiguredUpdateChannel`) are also exported for ad-hoc use outside the registered command. diff --git a/src/commands/update.test.ts b/src/commands/update.test.ts index 9efc250..a4e57fe 100644 --- a/src/commands/update.test.ts +++ b/src/commands/update.test.ts @@ -13,6 +13,11 @@ import { vi.mock('node:child_process', () => ({ spawn: vi.fn() })) +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, realpathSync: vi.fn(actual.realpathSync) } +}) + vi.mock('../config.js', async (importOriginal) => { const actual = await importOriginal() return { @@ -23,8 +28,10 @@ vi.mock('../config.js', async (importOriginal) => { }) const { spawn } = await import('node:child_process') +const { realpathSync } = await import('node:fs') const config = await import('../config.js') const mockSpawn = vi.mocked(spawn) +const mockRealpathSync = vi.mocked(realpathSync) const mockReadConfigOrThrow = vi.mocked(config.readConfigOrThrow) const mockUpdateConfigOrThrow = vi.mocked(config.updateConfigOrThrow) @@ -79,6 +86,9 @@ beforeEach(() => { mockReadConfigOrThrow.mockReset().mockResolvedValue({}) mockUpdateConfigOrThrow.mockReset().mockResolvedValue(undefined) mockSpawn.mockClear() + // Identity by default → resolved path has no `/Cellar/`, so the npm/pnpm + // cases stay brew-negative. Brew tests override this per-test. + mockRealpathSync.mockReset().mockImplementation((p) => String(p)) }) afterEach(() => { @@ -261,6 +271,51 @@ describe('update install flow', () => { }) }) +describe('update brew install flow', () => { + const CELLAR_PATH = '/opt/homebrew/Cellar/todoist-cli/1.1.0/bin/td' + + function createBrewProgram(formula?: string): Command { + const program = new Command() + program.name('td').exitOverride() + registerUpdateCommand(program, { ...BASE_OPTIONS, brewFormula: formula }) + return program + } + + it('runs `brew upgrade ` with inherited stdio instead of npm', async () => { + mockRealpathSync.mockReturnValue(CELLAR_PATH) + mockFetchOk('99.99.99') + mockSpawnExit() + await createBrewProgram('doist/tap/todoist-cli').parseAsync(['node', 'td', 'update']) + expect(mockSpawn).toHaveBeenCalledWith('brew', ['upgrade', 'doist/tap/todoist-cli'], { + stdio: 'inherit', + }) + }) + + it('throws UPDATE_INSTALL_FAILED on a non-zero brew exit', async () => { + mockRealpathSync.mockReturnValue(CELLAR_PATH) + mockFetchOk('99.99.99') + mockSpawnExit(1) + await expect( + createBrewProgram('doist/tap/todoist-cli').parseAsync(['node', 'td', 'update']), + ).rejects.toMatchObject({ + code: 'UPDATE_INSTALL_FAILED', + message: expect.stringContaining('brew exited with code 1'), + }) + }) + + it('refuses to fall back to npm when brew-installed without a configured formula', async () => { + mockRealpathSync.mockReturnValue(CELLAR_PATH) + mockFetchOk('99.99.99') + await expect( + createBrewProgram().parseAsync(['node', 'td', 'update']), + ).rejects.toMatchObject({ + code: 'UPDATE_INSTALL_FAILED', + hints: [expect.stringContaining('brew upgrade')], + }) + expect(mockSpawn).not.toHaveBeenCalled() + }) +}) + describe('update error paths', () => { it.each([ [ diff --git a/src/commands/update.ts b/src/commands/update.ts index acb66f9..8ed821e 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -1,4 +1,5 @@ import { spawn } from 'node:child_process' +import { realpathSync } from 'node:fs' import chalk from 'chalk' import type { Command } from 'commander' import { @@ -22,6 +23,12 @@ export type UpdateCommandOptions = { currentVersion: string /** Absolute path to the CLI's config file (use `getConfigPath(appName)`). */ configPath: string + /** + * Homebrew formula to `brew upgrade` when the CLI was installed via + * Homebrew, e.g. `'todoist-cli'` or a tapped `'doist/tap/todoist-cli'`. Set + * this on CLIs distributed through brew; omit for npm-only CLIs. + */ + brewFormula?: string /** Override the npm registry base URL. Default `'https://registry.npmjs.org'`. */ registryUrl?: string /** @@ -126,6 +133,24 @@ export async function getConfiguredUpdateChannel(configPath: string): Promise { + return new Promise((resolve, reject) => { + const child = spawn('brew', ['upgrade', formula], { + // Inherit so brew's own download/upgrade progress reaches the user + // (these can run for a while). Under machine-readable output, fall + // back to the npm-style pipe so brew's chatter can't corrupt the + // JSON/NDJSON stream on stdout. + stdio: quiet ? ['ignore', 'ignore', 'pipe'] : 'inherit', + }) + let stderr = '' + child.stderr?.on('data', (data: Buffer) => { + stderr += data.toString() + }) + child.on('error', reject) + child.on('close', (code) => resolve({ exitCode: code ?? 1, stderr })) + }) +} + function formatChannel(channel: UpdateChannel): string { return channel === 'pre-release' ? chalk.magenta('pre-release') : chalk.green('stable') } @@ -251,17 +297,40 @@ async function runUpdate(options: UpdateCommandOptions, cmd: UpdateCmdOptions): console.log(headline) } - const pm = detectPackageManager() + const brew = isBrewInstall() + if (brew && !options.brewFormula) { + throw new CliError( + 'UPDATE_INSTALL_FAILED', + 'Installed via Homebrew but no brew formula is configured.', + { hints: ['Update manually: brew upgrade '] }, + ) + } + const pm = brew ? null : detectPackageManager() + const quiet = Boolean(view.json || view.ndjson) let result: { exitCode: number; stderr: string } try { - result = await runWithSpinner( - options.withSpinner, - `Updating to v${latestVersion}${label}...`, - () => runInstall(pm, options.packageName, tag), - ) + if (brew) { + const formula = options.brewFormula as string + // brew prints its own progress, so skip the spinner unless we've + // silenced brew for machine-readable output. + result = quiet + ? await runWithSpinner( + options.withSpinner, + `Updating to v${latestVersion}${label}...`, + () => runBrewUpgrade(formula, true), + ) + : await runBrewUpgrade(formula, false) + } else { + result = await runWithSpinner( + options.withSpinner, + `Updating to v${latestVersion}${label}...`, + () => runInstall(pm as string, options.packageName, tag), + ) + } } catch (error) { if ( + !brew && error instanceof Error && 'code' in error && (error as { code?: unknown }).code === 'EACCES' @@ -279,7 +348,7 @@ async function runUpdate(options: UpdateCommandOptions, cmd: UpdateCmdOptions): if (result.exitCode !== 0) { throw new CliError( 'UPDATE_INSTALL_FAILED', - `${pm} exited with code ${result.exitCode}`, + `${brew ? 'brew' : pm} exited with code ${result.exitCode}`, result.stderr ? { hints: [result.stderr.trim()] } : {}, ) } @@ -340,8 +409,12 @@ async function runSwitch( * Commander program. The `update` action checks the npm registry for the * configured channel's dist-tag, compares against `currentVersion`, and shells * out to `npm i -g` (or `pnpm add -g` if pnpm is detected on the running - * script's path). `update switch` flips the persisted `update_channel` field - * between `'stable'` and `'pre-release'`. + * script's path). When the CLI was installed via Homebrew (the running script + * resolves into a brew Cellar) it instead runs `brew upgrade ` — + * note the brew formula can lag the npm publish, so the registry may report an + * update while `brew upgrade` is a no-op until the formula is bumped. + * `update switch` flips the persisted `update_channel` field between `'stable'` + * and `'pre-release'`. * * Errors as `CliError` (`INVALID_FLAGS`, `INVALID_UPDATE_CHANNEL`, * `UPDATE_CHECK_FAILED`, `UPDATE_INSTALL_FAILED`, or the canonical `CONFIG_*` @@ -363,6 +436,7 @@ async function runSwitch( * currentVersion: packageJson.version, * configPath: getConfigPath('todoist-cli'), * changelogCommandName: 'td changelog', + * brewFormula: 'doist/tap/todoist-cli', * withSpinner, * }) * ``` From 6689cafd6fe990063921b977518476672d409446 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Thu, 21 May 2026 18:40:16 +0100 Subject: [PATCH 2/3] refactor(update): address doistbot review on brew support - Report a neutral brew result instead of claiming the npm dist-tag version was installed (brew owns versioning and can be a no-op when the formula lags). - Fail fast on a brew install with no configured formula, before the registry round-trip. - Consolidate the spinner call and extract a shared spawn+capture helper. - Pin process.platform in brew tests for cross-OS determinism; add a quiet/JSON brew case asserting the piped stdio fallback. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/commands/update.test.ts | 45 ++++++++++++- src/commands/update.ts | 127 +++++++++++++++++++----------------- 2 files changed, 111 insertions(+), 61 deletions(-) diff --git a/src/commands/update.test.ts b/src/commands/update.test.ts index a4e57fe..bad6038 100644 --- a/src/commands/update.test.ts +++ b/src/commands/update.test.ts @@ -273,6 +273,7 @@ describe('update install flow', () => { describe('update brew install flow', () => { const CELLAR_PATH = '/opt/homebrew/Cellar/todoist-cli/1.1.0/bin/td' + const realPlatform = process.platform function createBrewProgram(formula?: string): Command { const program = new Command() @@ -281,7 +282,17 @@ describe('update brew install flow', () => { return program } - it('runs `brew upgrade ` with inherited stdio instead of npm', async () => { + beforeEach(() => { + // isBrewInstall() short-circuits on win32, bypassing the realpathSync + // mock; pin a posix platform so these tests are deterministic on any host. + Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true }) + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: realPlatform, configurable: true }) + }) + + it('runs `brew upgrade ` with inherited stdio and reports a neutral result', async () => { mockRealpathSync.mockReturnValue(CELLAR_PATH) mockFetchOk('99.99.99') mockSpawnExit() @@ -289,6 +300,35 @@ describe('update brew install flow', () => { expect(mockSpawn).toHaveBeenCalledWith('brew', ['upgrade', 'doist/tap/todoist-cli'], { stdio: 'inherit', }) + // brew owns its versioning, so the success line must not claim the npm + // dist-tag version was installed (the pre-install headline may still + // mention it as the available target). + expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('brew upgrade complete')) + expect(consoleSpy).not.toHaveBeenCalledWith(expect.stringContaining('Updated to v')) + }) + + it('pipes brew stdout under --json so its chatter cannot corrupt the stream', async () => { + mockRealpathSync.mockReturnValue(CELLAR_PATH) + mockFetchOk('99.99.99') + mockSpawnExit() + await createBrewProgram('doist/tap/todoist-cli').parseAsync([ + 'node', + 'td', + 'update', + '--json', + ]) + expect(mockSpawn).toHaveBeenCalledWith('brew', ['upgrade', 'doist/tap/todoist-cli'], { + stdio: ['ignore', 'ignore', 'pipe'], + }) + const payloads = consoleSpy.mock.calls.map((call: unknown[]) => + JSON.parse(call[0] as string), + ) + expect(payloads).toContainEqual({ + currentVersion: '1.0.0', + channel: 'stable', + installed: true, + via: 'brew', + }) }) it('throws UPDATE_INSTALL_FAILED on a non-zero brew exit', async () => { @@ -303,7 +343,7 @@ describe('update brew install flow', () => { }) }) - it('refuses to fall back to npm when brew-installed without a configured formula', async () => { + it('fails fast (before any registry call) when brew-installed without a formula', async () => { mockRealpathSync.mockReturnValue(CELLAR_PATH) mockFetchOk('99.99.99') await expect( @@ -312,6 +352,7 @@ describe('update brew install flow', () => { code: 'UPDATE_INSTALL_FAILED', hints: [expect.stringContaining('brew upgrade')], }) + expect(fetch).not.toHaveBeenCalled() expect(mockSpawn).not.toHaveBeenCalled() }) }) diff --git a/src/commands/update.ts b/src/commands/update.ts index 8ed821e..1ee86e2 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -1,4 +1,4 @@ -import { spawn } from 'node:child_process' +import { spawn, type SpawnOptions } from 'node:child_process' import { realpathSync } from 'node:fs' import chalk from 'chalk' import type { Command } from 'commander' @@ -162,24 +162,13 @@ function detectPackageManager(): 'npm' | 'pnpm' { return haystack.includes('pnpm') ? 'pnpm' : 'npm' } -function runInstall( - pm: string, - packageName: string, - tag: string, +function spawnCapture( + command: string, + args: string[], + options: SpawnOptions, ): Promise<{ exitCode: number; stderr: string }> { - const command = pm === 'pnpm' ? 'add' : 'install' return new Promise((resolve, reject) => { - const child = spawn(pm, [command, '-g', `${packageName}@${tag}`], { - // Ignore stdout so a chatty install can't deadlock by filling an - // unread pipe buffer; keep stderr piped so we can surface the tail - // in the CliError hint on a non-zero exit. - stdio: ['ignore', 'ignore', 'pipe'], - // npm/pnpm on Windows are `.cmd` shims that spawn() can't resolve - // without the shell. Safe to enable here because every argv element - // is library-controlled (literal command, fixed flags, validated - // package name + tag). - shell: process.platform === 'win32', - }) + const child = spawn(command, args, options) let stderr = '' child.stderr?.on('data', (data: Buffer) => { stderr += data.toString() @@ -189,24 +178,35 @@ function runInstall( }) } +function runInstall( + pm: string, + packageName: string, + tag: string, +): Promise<{ exitCode: number; stderr: string }> { + const command = pm === 'pnpm' ? 'add' : 'install' + return spawnCapture(pm, [command, '-g', `${packageName}@${tag}`], { + // Ignore stdout so a chatty install can't deadlock by filling an unread + // pipe buffer; keep stderr piped so we can surface the tail in the + // CliError hint on a non-zero exit. + stdio: ['ignore', 'ignore', 'pipe'], + // npm/pnpm on Windows are `.cmd` shims that spawn() can't resolve + // without the shell. Safe to enable here because every argv element is + // library-controlled (literal command, fixed flags, validated package + // name + tag). + shell: process.platform === 'win32', + }) +} + function runBrewUpgrade( formula: string, quiet: boolean, ): Promise<{ exitCode: number; stderr: string }> { - return new Promise((resolve, reject) => { - const child = spawn('brew', ['upgrade', formula], { - // Inherit so brew's own download/upgrade progress reaches the user - // (these can run for a while). Under machine-readable output, fall - // back to the npm-style pipe so brew's chatter can't corrupt the - // JSON/NDJSON stream on stdout. - stdio: quiet ? ['ignore', 'ignore', 'pipe'] : 'inherit', - }) - let stderr = '' - child.stderr?.on('data', (data: Buffer) => { - stderr += data.toString() - }) - child.on('error', reject) - child.on('close', (code) => resolve({ exitCode: code ?? 1, stderr })) + return spawnCapture('brew', ['upgrade', formula], { + // Inherit so brew's own download/upgrade progress reaches the user + // (these can run for a while). Under machine-readable output, fall back + // to the npm-style pipe so brew's chatter can't corrupt the JSON/NDJSON + // stream on stdout. + stdio: quiet ? ['ignore', 'ignore', 'pipe'] : 'inherit', }) } @@ -249,6 +249,17 @@ async function runUpdate(options: UpdateCommandOptions, cmd: UpdateCmdOptions): const tag = getInstallTag(channel) const label = channelLabel(channel) + // Fail fast on a guaranteed-broken install path before spending a registry + // round-trip. `--check` never installs, so a missing formula is fine there. + const brew = isBrewInstall() + if (!cmd.check && brew && !options.brewFormula) { + throw new CliError( + 'UPDATE_INSTALL_FAILED', + 'Installed via Homebrew but no brew formula is configured.', + { hints: ['Update manually: brew upgrade '] }, + ) + } + let latestVersion: string try { latestVersion = await runWithSpinner( @@ -297,37 +308,25 @@ async function runUpdate(options: UpdateCommandOptions, cmd: UpdateCmdOptions): console.log(headline) } - const brew = isBrewInstall() - if (brew && !options.brewFormula) { - throw new CliError( - 'UPDATE_INSTALL_FAILED', - 'Installed via Homebrew but no brew formula is configured.', - { hints: ['Update manually: brew upgrade '] }, - ) - } const pm = brew ? null : detectPackageManager() const quiet = Boolean(view.json || view.ndjson) + const task = () => + brew + ? runBrewUpgrade(options.brewFormula as string, quiet) + : runInstall(pm as string, options.packageName, tag) + // brew prints its own progress, so skip the spinner for it unless we've + // silenced its output for machine-readable mode. + const useSpinner = !brew || quiet let result: { exitCode: number; stderr: string } try { - if (brew) { - const formula = options.brewFormula as string - // brew prints its own progress, so skip the spinner unless we've - // silenced brew for machine-readable output. - result = quiet - ? await runWithSpinner( - options.withSpinner, - `Updating to v${latestVersion}${label}...`, - () => runBrewUpgrade(formula, true), - ) - : await runBrewUpgrade(formula, false) - } else { - result = await runWithSpinner( - options.withSpinner, - `Updating to v${latestVersion}${label}...`, - () => runInstall(pm as string, options.packageName, tag), - ) - } + result = useSpinner + ? await runWithSpinner( + options.withSpinner, + `Updating to v${latestVersion}${label}...`, + task, + ) + : await task() } catch (error) { if ( !brew && @@ -353,8 +352,18 @@ async function runUpdate(options: UpdateCommandOptions, cmd: UpdateCmdOptions): ) } - emitView(view, { currentVersion, latestVersion, channel, installed: true }, () => { - const lines = [`${chalk.green('✓')} Updated to v${latestVersion}${label}`] + // brew owns its own versioning and can resolve to a different (or + // unchanged) version than the npm dist-tag we checked — so don't claim + // `latestVersion` was installed. Report a neutral result instead. + const summary = brew + ? { currentVersion, channel, installed: true, via: 'brew' as const } + : { currentVersion, latestVersion, channel, installed: true } + emitView(view, summary, () => { + const lines = [ + brew + ? `${chalk.green('✓')} brew upgrade complete${label}` + : `${chalk.green('✓')} Updated to v${latestVersion}${label}`, + ] if (channel === 'stable' && options.changelogCommandName) { lines.push( `${chalk.dim(' Run')} ${chalk.cyan(options.changelogCommandName)} ${chalk.dim('to see what changed')}`, From c0a75b234e654078a42f7d8c62b7459440558d58 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Thu, 21 May 2026 18:57:35 +0100 Subject: [PATCH 3/3] refactor(update): accurate brew install status + stable result schema Round 2 of doistbot review: - Read the on-disk version after `brew upgrade` (brew list --versions) and derive `installed` from it, so a no-op upgrade (formula lagging npm) no longer reports installed: true. Expose the applied version as `installedVersion`. - Stable success envelope across installers: always carry `latestVersion` plus a `via` discriminator (npm | pnpm | brew) instead of dropping keys on brew. - Evaluate `detectPackageManager()` unconditionally to drop the nullable union and `pm as string` cast; consolidate the summary/lines branches. - Tests reuse `createProgram(overrides)`; add coverage for the no-op vs changed brew result, formula-set-but-npm-managed fallback, and brew spinner threading. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/commands/update.test.ts | 154 +++++++++++++++++++++++++----------- src/commands/update.ts | 55 ++++++++++--- 2 files changed, 153 insertions(+), 56 deletions(-) diff --git a/src/commands/update.test.ts b/src/commands/update.test.ts index bad6038..19d3d71 100644 --- a/src/commands/update.test.ts +++ b/src/commands/update.test.ts @@ -42,10 +42,10 @@ const BASE_OPTIONS: UpdateCommandOptions = { changelogCommandName: 'td changelog', } -function createProgram(): Command { +function createProgram(overrides?: Partial): Command { const program = new Command() program.name('td').exitOverride() - registerUpdateCommand(program, BASE_OPTIONS) + registerUpdateCommand(program, { ...BASE_OPTIONS, ...overrides }) return program } @@ -267,19 +267,35 @@ describe('update install flow', () => { latestVersion: '99.99.99', channel: 'stable', installed: true, + via: 'npm', }) }) }) describe('update brew install flow', () => { const CELLAR_PATH = '/opt/homebrew/Cellar/todoist-cli/1.1.0/bin/td' + const FORMULA = 'doist/tap/todoist-cli' const realPlatform = process.platform - function createBrewProgram(formula?: string): Command { - const program = new Command() - program.name('td').exitOverride() - registerUpdateCommand(program, { ...BASE_OPTIONS, brewFormula: formula }) - return program + // `brew upgrade` exits `upgradeExit`; the follow-up `brew list --versions` + // reports `listedVersion` on stdout (used to derive the installed result). + function mockBrew({ + upgradeExit = 0, + listedVersion, + }: { upgradeExit?: number; listedVersion?: string } = {}) { + mockSpawn.mockImplementation(((_cmd: string, args: readonly string[]) => ({ + stdout: { + on: vi.fn((event: string, cb: (chunk: Buffer) => void) => { + if (event === 'data' && args[0] === 'list' && listedVersion) { + cb(Buffer.from(`todoist-cli ${listedVersion}\n`)) + } + }), + }, + stderr: { on: vi.fn() }, + on: vi.fn((event: string, cb: (arg?: unknown) => void) => { + if (event === 'close') cb(args[0] === 'list' ? 0 : upgradeExit) + }), + })) as never) } beforeEach(() => { @@ -292,51 +308,57 @@ describe('update brew install flow', () => { Object.defineProperty(process, 'platform', { value: realPlatform, configurable: true }) }) - it('runs `brew upgrade ` with inherited stdio and reports a neutral result', async () => { + it('runs `brew upgrade ` with inherited stdio and a neutral success line', async () => { mockRealpathSync.mockReturnValue(CELLAR_PATH) mockFetchOk('99.99.99') - mockSpawnExit() - await createBrewProgram('doist/tap/todoist-cli').parseAsync(['node', 'td', 'update']) - expect(mockSpawn).toHaveBeenCalledWith('brew', ['upgrade', 'doist/tap/todoist-cli'], { - stdio: 'inherit', - }) - // brew owns its versioning, so the success line must not claim the npm - // dist-tag version was installed (the pre-install headline may still - // mention it as the available target). + mockBrew({ listedVersion: '1.1.0' }) + await createProgram({ brewFormula: FORMULA }).parseAsync(['node', 'td', 'update']) + expect(mockSpawn).toHaveBeenCalledWith('brew', ['upgrade', FORMULA], { stdio: 'inherit' }) + // The success line must not claim the npm dist-tag version was installed + // (the pre-install headline may still mention it as the available target). expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('brew upgrade complete')) expect(consoleSpy).not.toHaveBeenCalledWith(expect.stringContaining('Updated to v')) }) - it('pipes brew stdout under --json so its chatter cannot corrupt the stream', async () => { - mockRealpathSync.mockReturnValue(CELLAR_PATH) - mockFetchOk('99.99.99') - mockSpawnExit() - await createBrewProgram('doist/tap/todoist-cli').parseAsync([ - 'node', - 'td', - 'update', - '--json', - ]) - expect(mockSpawn).toHaveBeenCalledWith('brew', ['upgrade', 'doist/tap/todoist-cli'], { - stdio: ['ignore', 'ignore', 'pipe'], - }) - const payloads = consoleSpy.mock.calls.map((call: unknown[]) => - JSON.parse(call[0] as string), - ) - expect(payloads).toContainEqual({ - currentVersion: '1.0.0', - channel: 'stable', - installed: true, - via: 'brew', - }) - }) + it.each([ + ['version changed → installed', '1.1.0', true], + ['no-op (formula lags npm) → not installed', '1.0.0', false], + ])( + 'derives installed from the on-disk brew version, keeping a stable --json schema: %s', + async (_, listedVersion, installed) => { + mockRealpathSync.mockReturnValue(CELLAR_PATH) + mockFetchOk('99.99.99') + mockBrew({ listedVersion }) + await createProgram({ brewFormula: FORMULA }).parseAsync([ + 'node', + 'td', + 'update', + '--json', + ]) + // stdout piped (not inherited) under --json so brew can't corrupt the stream. + expect(mockSpawn).toHaveBeenCalledWith('brew', ['upgrade', FORMULA], { + stdio: ['ignore', 'ignore', 'pipe'], + }) + const payloads = consoleSpy.mock.calls.map((call: unknown[]) => + JSON.parse(call[0] as string), + ) + expect(payloads).toContainEqual({ + currentVersion: '1.0.0', + latestVersion: '99.99.99', + channel: 'stable', + installed, + via: 'brew', + installedVersion: listedVersion, + }) + }, + ) it('throws UPDATE_INSTALL_FAILED on a non-zero brew exit', async () => { mockRealpathSync.mockReturnValue(CELLAR_PATH) mockFetchOk('99.99.99') - mockSpawnExit(1) + mockBrew({ upgradeExit: 1 }) await expect( - createBrewProgram('doist/tap/todoist-cli').parseAsync(['node', 'td', 'update']), + createProgram({ brewFormula: FORMULA }).parseAsync(['node', 'td', 'update']), ).rejects.toMatchObject({ code: 'UPDATE_INSTALL_FAILED', message: expect.stringContaining('brew exited with code 1'), @@ -346,15 +368,59 @@ describe('update brew install flow', () => { it('fails fast (before any registry call) when brew-installed without a formula', async () => { mockRealpathSync.mockReturnValue(CELLAR_PATH) mockFetchOk('99.99.99') - await expect( - createBrewProgram().parseAsync(['node', 'td', 'update']), - ).rejects.toMatchObject({ + await expect(createProgram().parseAsync(['node', 'td', 'update'])).rejects.toMatchObject({ code: 'UPDATE_INSTALL_FAILED', hints: [expect.stringContaining('brew upgrade')], }) expect(fetch).not.toHaveBeenCalled() expect(mockSpawn).not.toHaveBeenCalled() }) + + it('uses npm/pnpm (not brew) when a formula is set but the install is not brew-managed', async () => { + // realpathSync defaults to identity (no `/Cellar/`), so this is a plain + // npm global install even though brewFormula is configured. + mockFetchOk('99.99.99') + mockSpawnExit() + await createProgram({ brewFormula: FORMULA }).parseAsync(['node', 'td', 'update']) + expect(mockSpawn).toHaveBeenCalledWith( + 'npm', + ['install', '-g', '@doist/todoist-cli@latest'], + { + stdio: ['ignore', 'ignore', 'pipe'], + shell: false, + }, + ) + expect(mockSpawn).not.toHaveBeenCalledWith('brew', expect.anything(), expect.anything()) + }) + + it.each([ + ['interactive: no install spinner', [] as string[], false], + ['--json: install spinner (stdout silenced)', ['--json'], true], + ])( + 'threads the install spinner for brew only when quiet (%s)', + async (_, flags, expectSpinner) => { + mockRealpathSync.mockReturnValue(CELLAR_PATH) + mockFetchOk('99.99.99') + mockBrew({ listedVersion: '1.1.0' }) + const withSpinner = vi.fn((_opts: SpinnerOptions, op: () => Promise) => op()) + const program = new Command() + program.name('td').exitOverride() + registerUpdateCommand(program, { + ...BASE_OPTIONS, + brewFormula: FORMULA, + withSpinner: withSpinner as unknown as UpdateCommandOptions['withSpinner'], + }) + await program.parseAsync(['node', 'td', 'update', ...flags]) + const installSpinner = expect.objectContaining({ + text: expect.stringContaining('Updating to v'), + }) + if (expectSpinner) { + expect(withSpinner).toHaveBeenCalledWith(installSpinner, expect.any(Function)) + } else { + expect(withSpinner).not.toHaveBeenCalledWith(installSpinner, expect.any(Function)) + } + }, + ) }) describe('update error paths', () => { diff --git a/src/commands/update.ts b/src/commands/update.ts index 1ee86e2..d128fb6 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -210,6 +210,24 @@ function runBrewUpgrade( }) } +function brewInstalledVersion(formula: string): Promise { + // `brew list --versions ` prints e.g. `todoist-cli 1.1.0`; the version + // is the last whitespace token. Tap-qualified formulae list under their leaf + // name, so strip the tap prefix. Best-effort — any failure yields undefined. + const leaf = formula.split('/').pop() ?? formula + return new Promise((resolve) => { + let stdout = '' + const child = spawn('brew', ['list', '--versions', leaf], { + stdio: ['ignore', 'pipe', 'ignore'], + }) + child.stdout?.on('data', (data: Buffer) => { + stdout += data.toString() + }) + child.on('error', () => resolve(undefined)) + child.on('close', () => resolve(stdout.trim().split(/\s+/).pop() || undefined)) + }) +} + function formatChannel(channel: UpdateChannel): string { return channel === 'pre-release' ? chalk.magenta('pre-release') : chalk.green('stable') } @@ -308,12 +326,12 @@ async function runUpdate(options: UpdateCommandOptions, cmd: UpdateCmdOptions): console.log(headline) } - const pm = brew ? null : detectPackageManager() + const pm = detectPackageManager() const quiet = Boolean(view.json || view.ndjson) const task = () => brew ? runBrewUpgrade(options.brewFormula as string, quiet) - : runInstall(pm as string, options.packageName, tag) + : runInstall(pm, options.packageName, tag) // brew prints its own progress, so skip the spinner for it unless we've // silenced its output for machine-readable mode. const useSpinner = !brew || quiet @@ -352,19 +370,32 @@ async function runUpdate(options: UpdateCommandOptions, cmd: UpdateCmdOptions): ) } - // brew owns its own versioning and can resolve to a different (or - // unchanged) version than the npm dist-tag we checked — so don't claim - // `latestVersion` was installed. Report a neutral result instead. - const summary = brew - ? { currentVersion, channel, installed: true, via: 'brew' as const } - : { currentVersion, latestVersion, channel, installed: true } + // For npm/pnpm we install the exact dist-tag, so the applied version is + // `latestVersion`. brew owns its own versioning and may be a no-op when the + // formula lags npm, so read the version brew actually has on disk and derive + // `installed` from it; if it can't be read, assume the upgrade applied. + const installedVersion = brew + ? await brewInstalledVersion(options.brewFormula as string) + : latestVersion + const installed = brew + ? installedVersion === undefined || installedVersion !== currentVersion + : true + // Stable schema across installers: always carry the registry target + // (`latestVersion`) plus a `via` discriminator; brew adds the applied + // `installedVersion` when known. + const summary = { + currentVersion, + latestVersion, + channel, + installed, + via: brew ? ('brew' as const) : pm, + ...(brew && installedVersion ? { installedVersion } : {}), + } emitView(view, summary, () => { const lines = [ - brew - ? `${chalk.green('✓')} brew upgrade complete${label}` - : `${chalk.green('✓')} Updated to v${latestVersion}${label}`, + `${chalk.green('✓')} ${brew ? 'brew upgrade complete' : `Updated to v${latestVersion}`}${label}`, ] - if (channel === 'stable' && options.changelogCommandName) { + if (installed && channel === 'stable' && options.changelogCommandName) { lines.push( `${chalk.dim(' Run')} ${chalk.cyan(options.changelogCommandName)} ${chalk.dim('to see what changed')}`, )