diff --git a/src/oclif/hooks/init/block-command-update-npm.ts b/src/oclif/hooks/init/block-command-update-npm.ts index 5db361188..be6dba974 100644 --- a/src/oclif/hooks/init/block-command-update-npm.ts +++ b/src/oclif/hooks/init/block-command-update-npm.ts @@ -2,7 +2,7 @@ import type {Hook} from '@oclif/core' import {execSync} from 'node:child_process' -import {isNpmGlobalInstall} from './update-notifier.js' +import {isNpmGlobalInstallCached} from './update-notifier.js' export type BlockCommandUpdateNpmDeps = { commandId: string | undefined @@ -17,10 +17,17 @@ export function handleBlockCommandUpdateNpm(deps: BlockCommandUpdateNpmDeps): vo } const hook: Hook<'init'> = async function (opts): Promise { + // This hook only acts when the user runs `brv update`, but previously it + // called `npm list -g byterover-cli` (a 3-5s subprocess on slow systems) + // on EVERY brv invocation, just so it could decide whether to error out + // when the command happened to be `update`. Gate the npm check on the + // commandId so every other command path skips it entirely. + if (opts.id !== 'update') return + handleBlockCommandUpdateNpm({ commandId: opts.id, errorFn: this.error.bind(this), - isNpmGlobalInstalled: isNpmGlobalInstall(execSync), + isNpmGlobalInstalled: isNpmGlobalInstallCached(this.config.root, execSync), }) } diff --git a/src/oclif/hooks/init/update-notifier.ts b/src/oclif/hooks/init/update-notifier.ts index c0551edfc..04a6665f0 100644 --- a/src/oclif/hooks/init/update-notifier.ts +++ b/src/oclif/hooks/init/update-notifier.ts @@ -2,6 +2,9 @@ import type {Hook} from '@oclif/core' import {confirm} from '@inquirer/prompts' import {execSync, spawn} from 'node:child_process' +import {mkdirSync, readFileSync, writeFileSync} from 'node:fs' +import {homedir} from 'node:os' +import {dirname, join} from 'node:path' import updateNotifier from 'update-notifier' import {checkForUpdatesSetting} from '../../lib/check-for-updates-setting.js' @@ -11,6 +14,75 @@ import {checkForUpdatesSetting} from '../../lib/check-for-updates-setting.js' */ export const UPDATE_CHECK_INTERVAL_MS = 1000 * 60 * 60 +/** + * How long to trust a cached `npm list -g` result before re-checking. + * Install method very rarely changes within a single CLI install lifetime, + * so 7 days is generous while still re-validating if the user e.g. moves + * from a global npm install to a tarball install. + */ +export const INSTALL_METHOD_CACHE_TTL_MS = 7 * 24 * 60 * 60 * 1000 + +/** + * Path to the install-method cache file. Uses XDG-style config dir under + * the user's home so it survives package upgrades and doesn't pollute the + * project's `.brv/` directory. + */ +export const getInstallMethodCachePath = (home: string = homedir()): string => + join(home, '.config', 'byterover-cli', 'install-method.json') + +type InstallMethodCacheEntry = { + /** Absolute path to the CLI's own dirname at cache time. Cache invalidates + * if the install moves (e.g. nvm version switch, different prefix). */ + cliPath: string + /** True iff `npm list -g byterover-cli` succeeded at cache time. */ + isNpmGlobal: boolean + /** Unix ms timestamp of the cache write. TTL is compared against this. */ + timestamp: number +} + +/** + * Read the cached install-method result, returning undefined when the cache + * is missing, malformed, expired, or for a different CLI install path. + * Never throws — a broken cache simply means we fall through to the live check. + */ +export const readInstallMethodCache = ( + cliPath: string, + cachePath: string = getInstallMethodCachePath(), + now: number = Date.now(), +): boolean | undefined => { + try { + const raw = readFileSync(cachePath, 'utf8') + const entry = JSON.parse(raw) as Partial + if (typeof entry.isNpmGlobal !== 'boolean') return undefined + if (entry.cliPath !== cliPath) return undefined + if (typeof entry.timestamp !== 'number') return undefined + if (now - entry.timestamp > INSTALL_METHOD_CACHE_TTL_MS) return undefined + return entry.isNpmGlobal + } catch { + return undefined + } +} + +/** + * Persist the install-method result to disk. Silently no-ops on any I/O + * failure — a missing cache entry just means the next invocation re-runs + * the live check, which is correct behaviour. + */ +export const writeInstallMethodCache = ( + cliPath: string, + isNpmGlobal: boolean, + cachePath: string = getInstallMethodCachePath(), + now: number = Date.now(), +): void => { + try { + mkdirSync(dirname(cachePath), {recursive: true}) + const entry: InstallMethodCacheEntry = {cliPath, isNpmGlobal, timestamp: now} + writeFileSync(cachePath, JSON.stringify(entry), 'utf8') + } catch { + // best-effort cache; never break the user's CLI invocation over this + } +} + /** * Narrowed notifier type for dependency injection */ @@ -35,7 +107,17 @@ export type UpdateNotifierDeps = { /** * Check whether byterover-cli is installed as a npm global package. - * @param execSyncFn + * + * Calling `npm list -g ` spawns a subprocess that walks the entire + * global node_modules tree. On a fast desktop with an SSD this is ~300ms; + * on slower environments (older systems, Termux/Android, network-mounted + * homedirs, CI runners with cold caches) it routinely takes 3-5 seconds. + * Since the install method virtually never changes between invocations, + * we cache the result on disk (see {@link readInstallMethodCache}) and + * only re-check after {@link INSTALL_METHOD_CACHE_TTL_MS} elapses or the + * CLI install location changes. + * + * @param execSyncFn - Injected for testability. * @returns false for other installation methods. */ export const isNpmGlobalInstall = (execSyncFn: typeof execSync): boolean => { @@ -47,6 +129,23 @@ export const isNpmGlobalInstall = (execSyncFn: typeof execSync): boolean => { } } +/** + * Cached wrapper around {@link isNpmGlobalInstall}. Used by the init hook to + * avoid paying the cost of an `npm list -g` subprocess on every CLI start. + */ +export const isNpmGlobalInstallCached = ( + cliPath: string, + execSyncFn: typeof execSync = execSync, + cachePath: string = getInstallMethodCachePath(), + now: number = Date.now(), +): boolean => { + const cached = readInstallMethodCache(cliPath, cachePath, now) + if (cached !== undefined) return cached + const live = isNpmGlobalInstall(execSyncFn) + writeInstallMethodCache(cliPath, live, cachePath, now) + return live +} + /** * Resolves whether the init hook should run the update check at all. * @@ -54,12 +153,23 @@ export const isNpmGlobalInstall = (execSyncFn: typeof execSync): boolean => { * - The `update.checkForUpdates` setting is explicitly `false`. * - The command being invoked is itself `update` (anti-recursion). * - `BRV_ENV` is `development` (dev installs handle updates manually). + * - `BRV_SKIP_UPDATE_CHECK` is set to any truthy value (escape hatch for + * CI / scripted invocations that want zero startup overhead). + * - `stdout` is not a TTY. The interactive confirm prompt cannot run in + * non-interactive contexts (piped output, CI, scripts, daemons), so the + * hook would silently no-op anyway after paying the full `npm list -g` + * cost. Bailing here saves that cost on every non-TTY invocation. * * Returns `true` otherwise (default behaviour). */ -export function shouldRunUpdateCheck(args: {commandId: string | undefined}): boolean { +export function shouldRunUpdateCheck(args: { + commandId: string | undefined + isTTY?: boolean +}): boolean { if (args.commandId === 'update') return false if (process.env.BRV_ENV === 'development') return false + if (process.env.BRV_SKIP_UPDATE_CHECK) return false + if (args.isTTY === false) return false return checkForUpdatesSetting() } @@ -114,18 +224,30 @@ export async function handleUpdateNotification(deps: UpdateNotifierDeps): Promis } const hook: Hook<'init'> = async function (opts): Promise { - if (!shouldRunUpdateCheck({commandId: opts.id})) return + const isTTY = process.stdout.isTTY ?? false + + // Cheap short-circuits FIRST, before any subprocess or filesystem I/O. + // Previously every invocation paid the cost of `npm list -g byterover-cli` + // (3-5 seconds on slower systems) even when the hook would no-op anyway + // because stdout wasn't a TTY or the user had opted out. + if (!shouldRunUpdateCheck({commandId: opts.id, isTTY})) return + + // Use the cached install-method check so we don't spawn `npm list -g` on + // every CLI invocation. The cache is keyed on the CLI's own install path + // and invalidates after 7 days, so a user moving installs (e.g. nvm + // version switch) re-validates on the next run. + const cliPath = this.config.root + const isNpmGlobalInstalled = isNpmGlobalInstallCached(cliPath, execSync) const pkgInfo = {name: this.config.name, version: this.config.version} const notifier = updateNotifier({pkg: pkgInfo, updateCheckInterval: UPDATE_CHECK_INTERVAL_MS}) - const isNpmGlobalInstalled = isNpmGlobalInstall(execSync) await handleUpdateNotification({ confirmPrompt: confirm, execSyncFn: execSync, exitFn: process.exit, isNpmGlobalInstalled, - isTTY: process.stdout.isTTY ?? false, + isTTY, log: this.log.bind(this), notifier, spawnRestartFn: () => diff --git a/test/hooks/init/update-notifier.test.ts b/test/hooks/init/update-notifier.test.ts index 96a3e2f17..78e3948a8 100644 --- a/test/hooks/init/update-notifier.test.ts +++ b/test/hooks/init/update-notifier.test.ts @@ -1,5 +1,5 @@ import {expect} from 'chai' -import {mkdtempSync, rmSync, writeFileSync} from 'node:fs' +import {mkdtempSync, readFileSync, rmSync, writeFileSync} from 'node:fs' import {tmpdir} from 'node:os' import {join} from 'node:path' import * as sinon from 'sinon' @@ -8,9 +8,13 @@ import type {NarrowedUpdateNotifier, UpdateNotifierDeps} from '../../../src/ocli import { handleUpdateNotification, + INSTALL_METHOD_CACHE_TTL_MS, isNpmGlobalInstall, + isNpmGlobalInstallCached, + readInstallMethodCache, shouldRunUpdateCheck, UPDATE_CHECK_INTERVAL_MS, + writeInstallMethodCache, } from '../../../src/oclif/hooks/init/update-notifier.js' describe('update-notifier hook', () => { @@ -257,6 +261,51 @@ describe('update-notifier hook', () => { }) }) + describe('shouldRunUpdateCheck (new short-circuits)', () => { + let priorBrvEnv: string | undefined + let priorSkipFlag: string | undefined + let tempDir: string + let priorHome: string | undefined + + beforeEach(() => { + priorBrvEnv = process.env.BRV_ENV + priorSkipFlag = process.env.BRV_SKIP_UPDATE_CHECK + delete process.env.BRV_ENV + delete process.env.BRV_SKIP_UPDATE_CHECK + tempDir = mkdtempSync(join(tmpdir(), 'brv-test-')) + priorHome = process.env.HOME + process.env.HOME = tempDir + }) + + afterEach(() => { + if (priorBrvEnv === undefined) delete process.env.BRV_ENV + else process.env.BRV_ENV = priorBrvEnv + if (priorSkipFlag === undefined) delete process.env.BRV_SKIP_UPDATE_CHECK + else process.env.BRV_SKIP_UPDATE_CHECK = priorSkipFlag + if (priorHome === undefined) delete process.env.HOME + else process.env.HOME = priorHome + rmSync(tempDir, {force: true, recursive: true}) + }) + + it('returns false when BRV_SKIP_UPDATE_CHECK is set (escape hatch)', () => { + process.env.BRV_SKIP_UPDATE_CHECK = '1' + expect(shouldRunUpdateCheck({commandId: 'status', isTTY: true})).to.equal(false) + }) + + it('returns false when isTTY is false (non-interactive contexts)', () => { + expect(shouldRunUpdateCheck({commandId: 'status', isTTY: false})).to.equal(false) + }) + + it('returns true when isTTY is omitted (back-compat with callers that did not pass it)', () => { + expect(shouldRunUpdateCheck({commandId: 'status'})).to.equal(true) + }) + + it('still returns false when BRV_ENV=development even with isTTY=true', () => { + process.env.BRV_ENV = 'development' + expect(shouldRunUpdateCheck({commandId: 'status', isTTY: true})).to.equal(false) + }) + }) + describe('isNpmGlobalInstall', () => { it('should return true when npm list succeeds', () => { const execSyncStub = sinon.stub().returns(Buffer.from('')) @@ -270,4 +319,162 @@ describe('update-notifier hook', () => { expect(isNpmGlobalInstall(execSyncStub as unknown as typeof import('node:child_process').execSync)).to.be.false }) }) + + describe('install-method cache', () => { + let tempDir: string + let cachePath: string + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), 'brv-cache-test-')) + cachePath = join(tempDir, 'install-method.json') + }) + + afterEach(() => { + rmSync(tempDir, {force: true, recursive: true}) + }) + + describe('readInstallMethodCache', () => { + it('returns undefined when the cache file does not exist', () => { + expect(readInstallMethodCache('/some/cli/path', cachePath)).to.equal(undefined) + }) + + it('returns undefined when the cache file is malformed JSON', () => { + writeFileSync(cachePath, '{not json') + expect(readInstallMethodCache('/some/cli/path', cachePath)).to.equal(undefined) + }) + + it('returns undefined when cliPath in the cache does not match the caller', () => { + writeFileSync( + cachePath, + JSON.stringify({cliPath: '/other/cli/path', isNpmGlobal: true, timestamp: Date.now()}), + ) + expect(readInstallMethodCache('/some/cli/path', cachePath)).to.equal(undefined) + }) + + it('returns undefined when the entry has expired beyond the TTL', () => { + const old = Date.now() - INSTALL_METHOD_CACHE_TTL_MS - 1 + writeFileSync( + cachePath, + JSON.stringify({cliPath: '/some/cli/path', isNpmGlobal: true, timestamp: old}), + ) + expect(readInstallMethodCache('/some/cli/path', cachePath, Date.now())).to.equal(undefined) + }) + + it('returns the cached isNpmGlobal value when fresh and matching', () => { + const now = Date.now() + writeFileSync( + cachePath, + JSON.stringify({cliPath: '/some/cli/path', isNpmGlobal: true, timestamp: now}), + ) + expect(readInstallMethodCache('/some/cli/path', cachePath, now)).to.equal(true) + }) + + it('returns false (not undefined) when the cached value is explicitly false', () => { + const now = Date.now() + writeFileSync( + cachePath, + JSON.stringify({cliPath: '/some/cli/path', isNpmGlobal: false, timestamp: now}), + ) + expect(readInstallMethodCache('/some/cli/path', cachePath, now)).to.equal(false) + }) + }) + + describe('writeInstallMethodCache', () => { + it('writes a parseable cache entry with the expected shape', () => { + const now = Date.now() + writeInstallMethodCache('/some/cli/path', true, cachePath, now) + const parsed = JSON.parse(readFileSync(cachePath, 'utf8')) + expect(parsed).to.deep.equal({cliPath: '/some/cli/path', isNpmGlobal: true, timestamp: now}) + }) + + it('creates parent directories that do not yet exist', () => { + const nested = join(tempDir, 'deeply', 'nested', 'install-method.json') + writeInstallMethodCache('/some/cli/path', false, nested) + const parsed = JSON.parse(readFileSync(nested, 'utf8')) + expect(parsed.isNpmGlobal).to.equal(false) + }) + + it('silently no-ops when the path is not writable (does not throw)', () => { + // NUL byte in the path forces mkdirSync / writeFileSync to fail + // synchronously on every platform. The point is just to verify the + // try/catch around the cache write does not propagate the error. + const bad = '/tmp/brv-test-\u0000/install-method.json' + expect(() => { + writeInstallMethodCache('/some/cli/path', true, bad) + }).to.not.throw() + }) + }) + + describe('isNpmGlobalInstallCached', () => { + it('skips the live execSync call on cache hit', () => { + const now = Date.now() + writeFileSync( + cachePath, + JSON.stringify({cliPath: '/some/cli/path', isNpmGlobal: true, timestamp: now}), + ) + const execSyncStub = sinon.stub() + const result = isNpmGlobalInstallCached( + '/some/cli/path', + execSyncStub as unknown as typeof import('node:child_process').execSync, + cachePath, + now, + ) + expect(result).to.equal(true) + expect(execSyncStub.called).to.equal(false) + }) + + it('runs the live execSync call on cache miss and persists the result', () => { + const execSyncStub = sinon.stub().returns(Buffer.from('')) + const now = Date.now() + const result = isNpmGlobalInstallCached( + '/some/cli/path', + execSyncStub as unknown as typeof import('node:child_process').execSync, + cachePath, + now, + ) + expect(result).to.equal(true) + expect(execSyncStub.calledOnce).to.equal(true) + const persisted = JSON.parse(readFileSync(cachePath, 'utf8')) + expect(persisted).to.deep.equal({cliPath: '/some/cli/path', isNpmGlobal: true, timestamp: now}) + }) + + it('runs the live execSync call when the cache is expired', () => { + const now = Date.now() + writeFileSync( + cachePath, + JSON.stringify({ + cliPath: '/some/cli/path', + isNpmGlobal: true, + timestamp: now - INSTALL_METHOD_CACHE_TTL_MS - 1, + }), + ) + const execSyncStub = sinon.stub().throws(new Error('not found')) + const result = isNpmGlobalInstallCached( + '/some/cli/path', + execSyncStub as unknown as typeof import('node:child_process').execSync, + cachePath, + now, + ) + expect(result).to.equal(false) + expect(execSyncStub.calledOnce).to.equal(true) + }) + + it('runs the live execSync call when cached cliPath differs (install moved)', () => { + const now = Date.now() + writeFileSync( + cachePath, + JSON.stringify({cliPath: '/old/cli/path', isNpmGlobal: true, timestamp: now}), + ) + const execSyncStub = sinon.stub().throws(new Error('not found')) + const result = isNpmGlobalInstallCached( + '/new/cli/path', + execSyncStub as unknown as typeof import('node:child_process').execSync, + cachePath, + now, + ) + expect(result).to.equal(false) + expect(execSyncStub.calledOnce).to.equal(true) + }) + }) + }) })