fix: stop forwarding unvalidated ni output to the shell in the CLI

packages/cli/package.json

@@ -46,8 +46,7 @@
     "globby": "^15.0.0",
     "oclif": "^4",
     "ora": "5.4.1",
-    "prettier": "^3.1.0",
-    "resolve-pkg": "^3.0.0"
+    "prettier": "^3.1.0"
   },
   "devDependencies": {
     "@types/degit": "^2.8.6",

packages/cli/src/commands/start.test.ts

@@ -0,0 +1,119 @@
+import fs from 'node:fs'
+import os from 'node:os'
+import path from 'node:path'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+
+const spawnMock = vi.hoisted(() => vi.fn())
+const spawnSyncMock = vi.hoisted(() => vi.fn())
+const resolvePackageManagerMock = vi.hoisted(() => vi.fn())
+
+vi.mock('node:child_process', () => ({
+  spawn: (...args: unknown[]) => spawnMock(...args),
+  spawnSync: (...args: unknown[]) => spawnSyncMock(...args),
+}))
+
+vi.mock('../utils/commands', () => ({
+  resolvePackageManager: (...args: unknown[]) =>
+    resolvePackageManagerMock(...args),
+}))
+
+import Start from './start'
+
+/** `Command.parse` is protected, so it has to be reached through a structural type. */
+type Parseable = { parse: () => Promise<{ args: { path: string } }> }
+
+describe('Start', () => {
+  let storeDir: string
+
+  async function runStart() {
+    const cmd = new Start([], {} as never)
+
+    vi.spyOn(cmd as unknown as Parseable, 'parse').mockResolvedValue({
+      args: { path: storeDir },
+    })
+
+    await cmd.run()
+  }
+
+  beforeEach(() => {
+    storeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'faststore-start-'))
+    fs.mkdirSync(path.join(storeDir, '.next'))
+    vi.clearAllMocks()
+    spawnSyncMock.mockReturnValue({ status: 0 })
+  })
+
+  afterEach(() => {
+    fs.rmSync(storeDir, { recursive: true, force: true })
+  })
+
+  it('spawns the package manager binary without a shell', async () => {
+    resolvePackageManagerMock.mockResolvedValue({
+      agent: 'yarn',
+      command: 'yarn',
+      argv: ['yarn'],
+    })
+
+    await runStart()
+
+    expect(resolvePackageManagerMock).toHaveBeenCalledWith(storeDir)
+    expect(spawnMock).toHaveBeenCalledWith(
+      'yarn',
+      ['next', 'start', path.join(storeDir, '.faststore'), '-p', '3000'],
+      { stdio: 'inherit' }
+    )
+  })
+
+  it('splits a Volta prefix into the spawn arguments', async () => {
+    resolvePackageManagerMock.mockResolvedValue({
+      agent: 'yarn',
+      command: 'volta run yarn',
+      argv: ['volta', 'run', 'yarn'],
+    })
+
+    await runStart()
+
+    expect(spawnMock).toHaveBeenCalledWith(
+      'volta',
+      [
+        'run',
+        'yarn',
+        'next',
+        'start',
+        path.join(storeDir, '.faststore'),
+        '-p',
+        '3000',
+      ],
+      { stdio: 'inherit' }
+    )
+  })
+
+  it('builds first when .next is missing, through a shell', async () => {
+    fs.rmSync(path.join(storeDir, '.next'), { recursive: true })
+    resolvePackageManagerMock.mockResolvedValue({
+      agent: 'yarn',
+      command: 'volta run yarn',
+      argv: ['volta', 'run', 'yarn'],
+    })
+
+    await runStart()
+
+    expect(spawnSyncMock).toHaveBeenCalledWith(
+      'volta run yarn faststore build',
+      { shell: true, stdio: 'inherit' }
+    )
+    expect(spawnMock).toHaveBeenCalled()
+  })
+
+  it('does not serve when the build fails', async () => {
+    fs.rmSync(path.join(storeDir, '.next'), { recursive: true })
+    resolvePackageManagerMock.mockResolvedValue({
+      agent: 'yarn',
+      command: 'yarn',
+      argv: ['yarn'],
+    })
+    spawnSyncMock.mockReturnValue({ status: 1 })
+
+    await expect(runStart()).rejects.toThrow('faststore build" failed')
+    expect(spawnMock).not.toHaveBeenCalled()
+  })
+})

packages/cli/src/commands/start.ts

@@ -2,7 +2,7 @@ import { Args, Command } from '@oclif/core'
 import { spawn, spawnSync } from 'node:child_process'
 import fsExtra from 'fs-extra'
 import path from 'node:path'
-import { getPreferredPackageManager } from '../utils/commands'
+import { resolvePackageManager } from '../utils/commands'
 import { getBasePath, withBasePath } from '../utils/directory'
 
 const { existsSync } = fsExtra
@@ -27,18 +27,26 @@ export default class Start extends Command {
     const basePath = getBasePath(args.path)
     const port = args.port ?? 3000
     const { getRoot, tmpDir } = withBasePath(basePath)
-    const packageManager = await getPreferredPackageManager()
+    const { command, argv } = await resolvePackageManager(basePath)
 
     if (!existsSync(path.join(getRoot(), '.next'))) {
-      spawnSync(`${packageManager} faststore build`, {
+      const build = spawnSync(`${command} faststore build`, {
         shell: true,
         stdio: 'inherit',
       })
+
+      if (build.status !== 0) {
+        throw new Error(
+          `"${command} faststore build" failed, so there is no build to serve.`
+        )
+      }
     }
 
+    const [bin, ...runnerArgs] = argv
+
     return spawn(
-      packageManager,
-      ['next', 'start', tmpDir, '-p', String(port)],
+      bin,
+      [...runnerArgs, 'next', 'start', tmpDir, '-p', String(port)],
       {
         stdio: 'inherit',
       }

packages/cli/src/utils/commands.test.ts

@@ -0,0 +1,201 @@
+import fs from 'node:fs'
+import os from 'node:os'
+import path from 'node:path'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import type { MockInstance } from 'vitest'
+import { cmdExists, detect, getVoltaPrefix } from '@antfu/ni'
+import {
+  NoAvailablePackageManagerError,
+  UnknownAgentError,
+  getPreferredPackageManager,
+  resolvePackageManager,
+} from './commands'
+import { logger } from './logger'
+
+vi.mock('@antfu/ni', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('@antfu/ni')>()),
+  detect: vi.fn(),
+  cmdExists: vi.fn(),
+  getVoltaPrefix: vi.fn(),
+}))
+
+const onlyAvailable =
+  (...available: string[]) =>
+  (cmd: string) =>
+    available.includes(cmd)
+
+describe('resolvePackageManager', () => {
+  let cwd: string
+  let warnMock: MockInstance<typeof console.warn>
+
+  beforeEach(() => {
+    cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'faststore-commands-'))
+    warnMock = vi.spyOn(logger, 'warn').mockImplementation(() => {})
+    vi.mocked(getVoltaPrefix).mockReturnValue('')
+    vi.mocked(cmdExists).mockImplementation(
+      onlyAvailable('yarn', 'npm', 'pnpm')
+    )
+  })
+
+  afterEach(() => {
+    warnMock.mockRestore()
+    vi.mocked(detect).mockReset()
+    fs.rmSync(cwd, { recursive: true, force: true })
+  })
+
+  it('never lets ni go interactive', async () => {
+    vi.mocked(detect).mockResolvedValue('yarn')
+
+    await resolvePackageManager(cwd)
+
+    expect(detect).toHaveBeenCalledWith(
+      expect.objectContaining({ programmatic: true })
+    )
+  })
+
+  it('returns the detected agent when its executable is available', async () => {
+    vi.mocked(detect).mockResolvedValue('pnpm')
+
+    await expect(resolvePackageManager(cwd)).resolves.toEqual({
+      agent: 'pnpm',
+      command: 'pnpm',
+      argv: ['pnpm'],
+    })
+    expect(warnMock).not.toHaveBeenCalled()
+  })
+
+  it('keeps the Volta prefix out of the agent id', async () => {
+    vi.mocked(detect).mockResolvedValue('yarn')
+    vi.mocked(getVoltaPrefix).mockReturnValue('volta run')
+
+    await expect(resolvePackageManager(cwd)).resolves.toEqual({
+      agent: 'yarn',
+      command: 'volta run yarn',
+      argv: ['volta', 'run', 'yarn'],
+    })
+  })
+
+  it('resolves versioned agents to their bare executable', async () => {
+    vi.mocked(detect).mockResolvedValue('yarn@berry')
+
+    await expect(resolvePackageManager(cwd)).resolves.toEqual({
+      agent: 'yarn@berry',
+      command: 'yarn',
+      argv: ['yarn'],
+    })
+  })
+
+  it('defaults to yarn when nothing is detected', async () => {
+    vi.mocked(detect).mockResolvedValue(null)
+
+    const { agent } = await resolvePackageManager(cwd)
+
+    expect(agent).toBe('yarn')
+  })
+
+  it('substitutes an available agent and reports the substitution', async () => {
+    vi.mocked(detect).mockResolvedValue('pnpm')
+    vi.mocked(cmdExists).mockImplementation(onlyAvailable('yarn', 'npm'))
+
+    const { agent, command } = await resolvePackageManager(cwd)
+
+    expect(agent).toBe('yarn')
+    expect(command).toBe('yarn')
+    expect(warnMock).toHaveBeenCalledWith(
+      expect.stringContaining('Detected "pnpm" but it is not installed')
+    )
+    expect(warnMock).toHaveBeenCalledWith(
+      expect.stringContaining('Using "yarn" instead')
+    )
+  })
+
+  it('falls back to npm when yarn is unavailable too', async () => {
+    vi.mocked(detect).mockResolvedValue('pnpm')
+    vi.mocked(cmdExists).mockImplementation(onlyAvailable('npm'))
+
+    const { agent } = await resolvePackageManager(cwd)
+
+    expect(agent).toBe('npm')
+  })
+
+  it('falls back to any known agent as a last resort', async () => {
+    vi.mocked(detect).mockResolvedValue('yarn')
+    vi.mocked(cmdExists).mockImplementation(onlyAvailable('bun'))
+
+    const { agent } = await resolvePackageManager(cwd)
+
+    expect(agent).toBe('bun')
+  })
+
+  it('throws instead of substituting when substitution is disabled', async () => {
+    vi.mocked(detect).mockResolvedValue('pnpm')
+    vi.mocked(cmdExists).mockImplementation(onlyAvailable('yarn', 'npm'))
+
+    await expect(
+      resolvePackageManager(cwd, { substitute: false })
+    ).rejects.toThrow(NoAvailablePackageManagerError)
+    expect(warnMock).not.toHaveBeenCalled()
+  })
+
+  it('resolves the detected agent normally when substitution is disabled', async () => {
+    vi.mocked(detect).mockResolvedValue('pnpm')
+
+    await expect(
+      resolvePackageManager(cwd, { substitute: false })
+    ).resolves.toMatchObject({ agent: 'pnpm' })
+  })
+
+  it('names the committed lockfiles when more than one is present', async () => {
+    fs.writeFileSync(path.join(cwd, 'yarn.lock'), '')
+    fs.writeFileSync(path.join(cwd, 'pnpm-lock.yaml'), '')
+    vi.mocked(detect).mockResolvedValue('pnpm')
+    vi.mocked(cmdExists).mockImplementation(onlyAvailable('yarn', 'npm'))
+
+    await resolvePackageManager(cwd)
+
+    const [message] = warnMock.mock.calls[0] as [string]
+    expect(message).toContain('More than one lockfile is committed')
+    expect(message).toContain('pnpm-lock.yaml')
+    expect(message).toContain('yarn.lock')
+  })
+
+  it('does not mention lockfiles when only one is committed', async () => {
+    fs.writeFileSync(path.join(cwd, 'pnpm-lock.yaml'), '')
+    vi.mocked(detect).mockResolvedValue('pnpm')
+    vi.mocked(cmdExists).mockImplementation(onlyAvailable('yarn', 'npm'))
+
+    await resolvePackageManager(cwd)
+
+    const [message] = warnMock.mock.calls[0] as [string]
+    expect(message).not.toContain('More than one lockfile is committed')
+  })
+
+  it('throws when no package manager is available at all', async () => {
+    vi.mocked(detect).mockResolvedValue('pnpm')
+    vi.mocked(cmdExists).mockImplementation(onlyAvailable())
+
+    await expect(resolvePackageManager(cwd)).rejects.toThrow(
+      NoAvailablePackageManagerError
+    )
+  })
+
+  it('throws instead of forwarding an unknown agent to a shell', async () => {
+    // Deliberately not a real package manager, so this keeps asserting the
+    // UnknownAgentError path even if `ni` grows support for a new agent.
+    vi.mocked(detect).mockResolvedValue('not-a-package-manager' as never)
+
+    await expect(resolvePackageManager(cwd)).rejects.toThrow(UnknownAgentError)
+  })
+})
+
+describe('getPreferredPackageManager', () => {
+  beforeEach(() => {
+    vi.mocked(cmdExists).mockImplementation(onlyAvailable('yarn'))
+    vi.mocked(getVoltaPrefix).mockReturnValue('volta run')
+    vi.mocked(detect).mockResolvedValue('yarn')
+  })
+
+  it('returns the shell-ready command', async () => {
+    await expect(getPreferredPackageManager()).resolves.toBe('volta run yarn')
+  })
+})