From 84eb88052aec6625d9deb5d535e7d182a08e1cbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1lber=20Laux?= Date: Wed, 29 Jul 2026 15:34:27 -0300 Subject: [PATCH 1/4] fix: stop forwarding unvalidated ni output to the shell in the CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getPreferredPackageManager() returned the raw stdout of `na ?` with no validation, and every caller interpolates that value into a shell command. `ni` ran in non-programmatic mode, where an agent that is not on PATH makes it render a confirm prompt to stdout instead of failing. Both the terminalLink non-TTY fallback and the "(y/N)" option contain "(", so the command became unparseable: sh -c "Would you like to globally install pnpm (https://…)? > (y/N) run build" -> /bin/sh: syntax error: unexpected "(" -> exit 2 The diagnostic that would have explained it goes to stderr, which spawnSync discards. Resolution now uses the ni library API with programmatic: true, so the prompt path is unreachable, and the agent is validated against ni's known agents before it can reach a shell. When the agent is not installed, the CLI logs what it detected and substitutes an available one instead of failing, so every build that works today keeps working. The return value is split into { agent, command, argv }, which also fixes two latent bugs: a `volta run` prefix broke start.ts, where the value was passed as argv[0] of a spawn without a shell, and dependencies.ts, which compared it with === 'npm'. Both now have regression tests. getPackageRootDir and getDepPackageJSON existed only to locate the `na` binary and nothing else imports them, so they are removed along with the now unused resolve-pkg dependency. Ref: FAS-1199 --- packages/cli/package.json | 3 +- packages/cli/src/commands/start.test.ts | 103 ++++++ packages/cli/src/commands/start.ts | 12 +- packages/cli/src/utils/commands.test.ts | 172 ++++++++++ packages/cli/src/utils/commands.ts | 142 +++++---- packages/cli/src/utils/dependencies.test.ts | 68 ++++ packages/cli/src/utils/dependencies.ts | 8 +- pnpm-lock.yaml | 3 - specs/package-manager-resolution.md | 332 ++++++++++++++++++++ 9 files changed, 774 insertions(+), 69 deletions(-) create mode 100644 packages/cli/src/commands/start.test.ts create mode 100644 packages/cli/src/utils/commands.test.ts create mode 100644 packages/cli/src/utils/dependencies.test.ts create mode 100644 specs/package-manager-resolution.md diff --git a/packages/cli/package.json b/packages/cli/package.json index fd01e64d1d..132f9b803b 100644 --- a/packages/cli/package.json +++ b/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", diff --git a/packages/cli/src/commands/start.test.ts b/packages/cli/src/commands/start.test.ts new file mode 100644 index 0000000000..5e8470a555 --- /dev/null +++ b/packages/cli/src/commands/start.test.ts @@ -0,0 +1,103 @@ +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() + }) + + 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(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' } + ) + }) +}) diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index 86096aedd4..4962503bad 100644 --- a/packages/cli/src/commands/start.ts +++ b/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,20 @@ 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() if (!existsSync(path.join(getRoot(), '.next'))) { - spawnSync(`${packageManager} faststore build`, { + spawnSync(`${command} faststore build`, { shell: true, stdio: 'inherit', }) } + const [bin, ...runnerArgs] = argv + return spawn( - packageManager, - ['next', 'start', tmpDir, '-p', String(port)], + bin, + [...runnerArgs, 'next', 'start', tmpDir, '-p', String(port)], { stdio: 'inherit', } diff --git a/packages/cli/src/utils/commands.test.ts b/packages/cli/src/utils/commands.test.ts new file mode 100644 index 0000000000..445d42fbaf --- /dev/null +++ b/packages/cli/src/utils/commands.test.ts @@ -0,0 +1,172 @@ +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()), + 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 + + 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('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 () => { + vi.mocked(detect).mockResolvedValue('deno' 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') + }) +}) diff --git a/packages/cli/src/utils/commands.ts b/packages/cli/src/utils/commands.ts index f006ef73de..2d0f75c730 100644 --- a/packages/cli/src/utils/commands.ts +++ b/packages/cli/src/utils/commands.ts @@ -1,71 +1,103 @@ -import fsExtra from 'fs-extra' -import { spawnSync } from 'node:child_process' +import { + LOCKS, + agents, + cmdExists, + detect, + getCommand, + getVoltaPrefix, +} from '@antfu/ni' +import type { Agent } from '@antfu/ni' +import chalk from 'chalk' +import { existsSync } from 'node:fs' import { join } from 'node:path' -import { pathToFileURL } from 'node:url' -import resolvePackage from 'resolve-pkg' - -const { existsSync } = fsExtra +import { logger } from './logger' + +export interface ResolvedPackageManager { + /** Agent id, validated against `ni`'s known agents. For comparisons, never for execution. */ + agent: Agent + /** Executable form for `spawn`/`spawnSync` with `shell: true`. May carry a Volta prefix. */ + command: string + /** Executable form for `spawn` without a shell. */ + argv: [string, ...string[]] +} -// Retrieves the package manager based on the developer lockfile, using `ni`. -export async function getPreferredPackageManager() { - let agent = 'yarn' // Default to Yarn - const binNA = join( - await getPackageRootDir('@antfu/ni'), - (await getDepPackageJSON('@antfu/ni'))?.bin?.['na'] ?? '' - ) +export class UnknownAgentError extends Error {} + +export class NoAvailablePackageManagerError extends Error {} + +const DEFAULT_AGENT: Agent = 'yarn' +const FALLBACK_AGENTS: Agent[] = ['yarn', 'npm'] + +/** + * Resolves the package manager to use for `cwd`. + * + * `programmatic: true` is what keeps `ni` from going interactive. Without it, an + * agent that is not installed makes `ni` render a confirm prompt to stdout, and + * callers interpolate this value straight into shell commands. + */ +export async function resolvePackageManager( + cwd: string = process.cwd() +): Promise { + const detected = (await detect({ programmatic: true, cwd })) ?? DEFAULT_AGENT + + if (!agents.includes(detected)) { + throw new UnknownAgentError( + `"${detected}" is not a known package manager. Expected one of: ${agents.join( + ', ' + )}.` + ) + } - if (!binNA || fsExtra.existsSync(binNA) == false) return agent + const agent = cmdExists(binOf(detected)) + ? detected + : substituteAgent(detected, cwd) - agent = spawnSync('node', [binNA, '?'], { encoding: 'utf-8' })?.stdout.trim() + const voltaPrefix = getVoltaPrefix() + const command = voltaPrefix ? `${voltaPrefix} ${binOf(agent)}` : binOf(agent) - return agent + return { agent, command, argv: command.split(' ') as [string, ...string[]] } } -export async function getPackageRootDir( - pkg: string, - cwd: string | undefined = process.cwd(), - depth = 30 -) { - let pkgPath = resolvePackage(pkg, { cwd }) - - if (!pkgPath) throw new Error(`Couldn't resolve package ${pkg}`) - - let pkgJson = await loadPackageJsonAt(pkgPath) - while (pkgJson?.name !== pkg && --depth > 0) { - pkgPath = join(pkgPath, '..') - pkgJson = await loadPackageJsonAt(join(pkgPath, '..')) - } - - if (pkgJson?.name !== pkg) - throw new Error(`Maximum depth search for package ${pkg} root exceed`) +/** Shell-ready form of {@link resolvePackageManager}, for callers that only run a command. */ +export async function getPreferredPackageManager( + cwd?: string +): Promise { + const { command } = await resolvePackageManager(cwd) - return pkgPath + return command } -async function loadPackageJsonAt(at?: string): Promise< - | undefined - | (Record & { - name: string - dependencies?: Record - peerDependencies?: Record - bin?: Record - }) -> { - const file = 'package.json', - location = (at?.endsWith(file) && at) || (at && join(at, file)) || false +function binOf(agent: Agent): string { + return getCommand(agent, 'agent') +} - if (location === false) - throw new Error(`Invalid searching of ${file} at ${at}`) +function substituteAgent(detected: Agent, cwd: string): Agent { + const available = FALLBACK_AGENTS.find((candidate) => + cmdExists(binOf(candidate)) + ) - if (!existsSync(location)) return + if (!available) { + throw new NoAvailablePackageManagerError( + `Detected "${detected}", which is not installed, and none of ${FALLBACK_AGENTS.join( + ', ' + )} is available either.` + ) + } - const content = await import(pathToFileURL(location).href, { - with: { type: 'json' }, - }) + const lockfiles = Object.keys(LOCKS).filter((lockfile) => + existsSync(join(cwd, lockfile)) + ) - return content.default ?? content ?? {} -} + logger.warn( + `${chalk.yellow( + 'warning' + )} - Detected "${detected}" but it is not installed in this environment. Using "${available}" instead.` + + (lockfiles.length > 1 + ? `\nMore than one lockfile is committed (${lockfiles.join( + ', ' + )}). Keep a single one so the package manager is unambiguous.` + : '') + ) -export async function getDepPackageJSON(pkg: string) { - return await loadPackageJsonAt(await getPackageRootDir(pkg)) + return available } diff --git a/packages/cli/src/utils/dependencies.test.ts b/packages/cli/src/utils/dependencies.test.ts new file mode 100644 index 0000000000..984c140f83 --- /dev/null +++ b/packages/cli/src/utils/dependencies.test.ts @@ -0,0 +1,68 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const resolvePackageManagerMock = vi.hoisted(() => vi.fn()) +const runCommandSyncMock = vi.hoisted(() => vi.fn()) + +vi.mock('./commands', () => ({ + resolvePackageManager: (...args: unknown[]) => + resolvePackageManagerMock(...args), +})) + +vi.mock('./runCommandSync', () => ({ + runCommandSync: (...args: unknown[]) => runCommandSyncMock(...args), +})) + +import { installDependencies } from './dependencies' + +async function install() { + await installDependencies({ + dependencies: ['preact@10.23.1'], + cwd: '/store', + errorMessage: 'failed to install Preact dependencies', + }) +} + +describe('installDependencies', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('uses "add" for agents other than npm', async () => { + resolvePackageManagerMock.mockResolvedValue({ + agent: 'yarn', + command: 'yarn', + }) + + await install() + + expect(runCommandSyncMock).toHaveBeenCalledWith( + expect.objectContaining({ cmd: 'yarn add preact@10.23.1' }) + ) + }) + + it('uses "install" for npm', async () => { + resolvePackageManagerMock.mockResolvedValue({ + agent: 'npm', + command: 'npm', + }) + + await install() + + expect(runCommandSyncMock).toHaveBeenCalledWith( + expect.objectContaining({ cmd: 'npm install preact@10.23.1' }) + ) + }) + + it('still recognises npm behind a Volta prefix', async () => { + resolvePackageManagerMock.mockResolvedValue({ + agent: 'npm', + command: 'volta run npm', + }) + + await install() + + expect(runCommandSyncMock).toHaveBeenCalledWith( + expect.objectContaining({ cmd: 'volta run npm install preact@10.23.1' }) + ) + }) +}) diff --git a/packages/cli/src/utils/dependencies.ts b/packages/cli/src/utils/dependencies.ts index 28f033a6c9..03a4caabcc 100644 --- a/packages/cli/src/utils/dependencies.ts +++ b/packages/cli/src/utils/dependencies.ts @@ -1,4 +1,4 @@ -import { getPreferredPackageManager } from './commands' +import { resolvePackageManager } from './commands' import { runCommandSync } from './runCommandSync' type InstallDependenciesOptions = { @@ -12,11 +12,11 @@ export async function installDependencies({ cwd, errorMessage, }: InstallDependenciesOptions) { - const packageManager = await getPreferredPackageManager() - const installCommand = packageManager === 'npm' ? 'install' : 'add' + const { agent, command } = await resolvePackageManager() + const installCommand = agent === 'npm' ? 'install' : 'add' runCommandSync({ - cmd: `${packageManager} ${installCommand} ${dependencies.join(' ')}`, + cmd: `${command} ${installCommand} ${dependencies.join(' ')}`, errorMessage, throws: 'error', cwd, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6d3f06f9fe..5adb516009 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -313,9 +313,6 @@ importers: prettier: specifier: ^3.1.0 version: 3.8.1 - resolve-pkg: - specifier: ^3.0.0 - version: 3.0.1 devDependencies: "@types/degit": specifier: ^2.8.6 diff --git a/specs/package-manager-resolution.md b/specs/package-manager-resolution.md new file mode 100644 index 0000000000..61eee2bb17 --- /dev/null +++ b/specs/package-manager-resolution.md @@ -0,0 +1,332 @@ +# Safe Package Manager Resolution + +> **Status**: Approved +> **Created**: 2026-07-29 + +> **References** +> - Incident: FastStore WebOps build failure on store account `cmsdev` — `/bin/sh: syntax error: unexpected "("`, exit code 2, during `yarn build` → `faststore build` +> - Upstream: `@antfu/ni@0.21.12` — `detect()` interactive fallback, `run()` `DEBUG_SIGN`, `getVoltaPrefix()` +> - Related infra: `dk-cicd-hub` → `dk_hub/dockerfiles/nextjs.Dockerfile` (the `builder` stage does not carry the package manager installed in `deps`) +> - Affected call sites: `packages/cli/src/utils/commands.ts:10`, `commands/build.ts:55`, `commands/dev.ts:64`, `commands/start.ts:30`, `commands/test.ts:33`, `commands/generate-graphql.ts:33`, `utils/dependencies.ts:15` + +## 1. Business Context + +### Problem Statement + +`getPreferredPackageManager()` resolves which package manager the CLI should use by spawning the `na` binary from `@antfu/ni` with the argument `?`, and returning its **raw stdout** with no validation: + +```ts +// packages/cli/src/utils/commands.ts:10-22 +agent = spawnSync('node', [binNA, '?'], { encoding: 'utf-8' })?.stdout.trim() +return agent +``` + +The returned string is then interpolated straight into a shell command by every caller — for example `spawnSync(`${packageManager} run build`, { shell: true, … })` in `build.ts`. + +`ni` is being invoked in **non-programmatic** mode. In that mode, when `detect()` resolves an agent whose binary is not on `PATH`, it does not fail — it renders an **interactive prompt to `process.stdout`**: + +- `terminalLink(agent, INSTALL_PAGE[agent])` has no hyperlink support in a non-TTY, so it falls back to `` `${text} (${url})` `` — e.g. `pnpm (https://pnpm.io/installation)`. +- The `confirm` prompt renders its `(y/N)` option, and `prompts` writes the rendered frame to `process.stdout`. + +Both contain `(`. That text becomes the value of `packageManager`, producing a command string the shell cannot parse: + +``` +sh -c "Would you like to globally install pnpm (https://…)? › (y/N) run build" + → /bin/sh: syntax error: unexpected "(" (exit 2) +``` + +The diagnostic that would have explained the problem — `[ni] Detected pnpm but it doesn't seem to be installed.` — is written to **stderr**, which the CLI discards because `spawnSync` pipes it. The operator only sees a shell syntax error with no relation to its cause. + +**How the incident was reached.** The store had both `pnpm-lock.yaml` and `yarn.lock` committed, and two components disagreed on precedence: + +| Component | Precedence order | Chose | +|---|---|---| +| `nextjs.Dockerfile` install chain | `yarn.lock`, `package-lock.json`, `pnpm-lock.yaml` | **yarn** — so `deps` installed with yarn and `pnpm` was never installed | +| `ni` `LOCKS` | `bun.lockb`, `pnpm-lock.yaml`, `yarn.lock`, `package-lock.json` | **pnpm** — not present in the `builder` stage | + +Removing `pnpm-lock.yaml` from the store made the build pass, which confirms the detection came from the lockfile and not from a `packageManager` field (that field takes precedence inside `detect()`). + +**Who is affected:** store maintainers whose repository detects an agent unavailable in the build image (typically two committed lockfiles, or a genuinely pnpm/bun store built by the WebOps image), and anyone debugging such a build — the failure is unattributable from the log. Local `faststore dev` / `start` / `test` are affected by the same path. + +### Goals + +Make package-manager resolution **safe by construction** and **self-explanatory when it cannot be satisfied**, without changing the outcome of any build that works today. + +**Branches in scope**: both `dev` (full change) and `origin/3.x` (widened fallback guard only, per Decision 5). Both are deliverables — the 3.x branch is where the observed incident happened, and no pipeline-side mitigation is planned, so 3.x stores are only reached by the backport. + +- An unvalidated string can never reach a shell command built by the CLI. +- When the detected agent is unavailable, the log states which agent was detected, where the detection came from, and what the CLI did about it. +- `ni` is never allowed to open an interactive prompt from inside the CLI. +- Callers that need an agent identity (for comparisons) and callers that need an executable command are served by distinct, unambiguous values. + +### User Stories + +#### US-1: Understandable failure + +- **Story**: As a store maintainer whose build fails, I want the log to name the package manager the CLI resolved and why it could not use it, so that I can act without reverse-engineering a shell error. +- **Acceptance Criteria**: + - **Given** a repository where `ni` detects an agent absent from `PATH`, **when** `faststore build` runs, **then** the log contains the detected agent, the detection source (`packageManager` field or lockfile name), and the agent actually used. + - **Given** the same repository, **when** the build proceeds, **then** no message emitted by the CLI contains an interactive prompt fragment such as `(y/N)`. + +#### US-2: No regression for builds that work today + +- **Story**: As a store maintainer whose build works today, I want this change to be invisible, so that a diagnostics improvement cannot break my pipeline. +- **Acceptance Criteria**: + - **Given** a repository whose detected agent is installed, **when** any CLI command resolves the package manager, **then** the executed command is byte-identical to the one executed before this change, including the `volta run` prefix when Volta is present. + - **Given** a repository with no lockfile and no `packageManager` field, **when** resolution runs, **then** the CLI uses its documented default rather than an agent chosen by `ni`'s interactive config. + +#### US-3: No unvalidated text in a shell command + +- **Story**: As a CLI maintainer, I want resolution to return a value drawn from a closed set, so that arbitrary text can never be interpolated into `spawnSync(…, { shell: true })`. +- **Acceptance Criteria**: + - **Given** any resolution outcome, **when** the value is produced, **then** its agent identity is a member of `ni`'s `agents` list. + - **Given** a resolution path that cannot produce a member of that set, **when** it is reached, **then** the CLI throws with an explicit message instead of returning the value. + +### Key Scenarios + +| # | Type | Pre-conditions | Steps | Expected result | +|---|---|---|---|---| +| 1 | Happy path | `yarn.lock` only; `yarn` on `PATH` | `faststore build` | Resolves `yarn`; runs `yarn run build` in `.faststore`; no new log lines | +| 2 | Happy path (Volta) | `yarn.lock` only; `volta` and `yarn` on `PATH` | `faststore build` | Shell command is `volta run yarn run build`; `spawn` without shell uses argv `['volta','run','yarn']` | +| 3 | Error → the incident | `pnpm-lock.yaml` + `yarn.lock`; only `yarn` on `PATH` | `faststore build` | Logs that `pnpm` was detected from `pnpm-lock.yaml` and is not installed, and that `yarn` is used instead; build proceeds; no shell syntax error | +| 4 | Error | `pnpm-lock.yaml` only; neither `pnpm` nor `yarn` on `PATH`; `npm` present | `faststore build` | Logs the detection and the substitution; resolves `npm`; `installDependencies` uses `npm install`, not `npm add` | +| 5 | Error | No usable agent on `PATH` at all | `faststore build` | Throws a named error listing the detected agent and the candidates tried; never emits a partial shell command | +| 6 | Edge | No lockfile, no `packageManager` field | `faststore build` | Resolves the documented default (`yarn` when available) deterministically; `ni`'s `defaultAgent: 'prompt'` never surfaces | +| 7 | Edge | `package.json` has `packageManager` naming an agent `ni` does not know | `faststore build` | Falls through to lockfile detection as `ni` already does; the unknown value is reported once, and never used as a command | +| 8 | Edge | `@antfu/ni` cannot be resolved from the CLI's install tree | any command | Falls back to the documented default with an explicit log line, matching today's early-return behaviour | + +### Functional Requirements + +- **FR-1**: Resolution MUST use `@antfu/ni`'s library API with `programmatic: true`, not the `na` binary. The binary exposes no way to disable the interactive fallback. +- **FR-2**: Resolution MUST validate the resolved agent against `ni`'s `agents` list before the value is used to build any command. +- **FR-3**: When the resolved agent's binary is not on `PATH`, resolution MUST log an explicit diagnostic and substitute the first available candidate, rather than failing. +- **FR-4**: When no candidate is available, resolution MUST throw a named error. It MUST NOT return a partially-formed or empty command. +- **FR-5**: Resolution MUST return the agent identity and the executable form as separate values, so callers stop pattern-matching a command string. +- **FR-6**: The `volta run` prefix MUST be preserved in the executable form, and MUST NOT leak into the agent identity. +- **FR-7**: Every existing call site MUST consume the value appropriate to how it spawns: the shell string for `shell: true`, the argv array for `spawn` without a shell, the agent identity for comparisons. + +### Non-Functional Requirements + +- **NFR-1**: No new runtime dependency. `@antfu/ni` is already a `dependencies` entry of `@faststore/cli`, pinned at `0.21.12` via `pnpm-workspace.yaml` `catalog:`. +- **NFR-2**: Resolution MUST NOT read from stdin or write an interactive prompt to stdout under any code path. +- **NFR-3**: Replacing the child-process spawn with an in-process call MUST NOT increase CLI startup cost; it removes one Node process per resolution. +- **NFR-4**: `packages/cli/src/utils/commands.test.ts` MUST cover FR-2 through FR-6, following the existing `src/utils/*.test.ts` Vitest pattern. +- **NFR-5**: Log messages MUST NOT include secrets, tokens, or `.env` values. Only the agent name, the detection source, and the substitution are reported. + +### Out of Scope + +The following were considered and deliberately excluded. All of them are pipeline-side: they would reach the whole store fleet on the next deploy, whereas this spec reaches each store only when it upgrades the CLI. That slower rollout is an **accepted trade-off** — there are no open reports of this failure beyond the one incident, which the store resolved by removing the redundant lockfile. + +- **Making the build image tolerant** (installing every package manager `ni` might detect into the `builder` stage of `nextjs.Dockerfile`). Would stop the crash fleet-wide and immediately, but a store with two lockfiles would then build with one manager over a tree installed by another — silently. This spec substitutes toward the manager that actually installed, and says so in the log. +- **Pre-build validation in the Tekton task** (flagging multiple lockfiles or a `packageManager` field inconsistent with the chosen lockfile). Would make the inconsistency legible fleet-wide, but as a warning it does not prevent the failure on an unpatched CLI, and as a hard failure it would break stores that build correctly today. +- **Changing which package manager a store installs with, or the lockfile precedence in `nextjs.Dockerfile`.** Would alter the production dependency tree of stores that build correctly today — unacceptable risk for a diagnostics fix. +- **Removing `shell: true` from the six call sites** in favour of argv arrays everywhere. FR-5 and FR-7 remove the injection surface at the source; converting every spawn is a larger refactor with no additional safety once the value set is closed. +- **Detecting or reporting multiple committed lockfiles as a store-level lint.** Worth doing, but it belongs to store scaffolding, not to command resolution. +- **Changing `@antfu/ni`'s version or replacing it with `package-manager-detector`.** + +--- + +## 2. Arch Decisions + +### Proposed Solution + +Replace the "spawn `na ?` and trust stdout" strategy with an in-process resolution built on `@antfu/ni`'s public API, returning a validated, structured value. + +`resolvePackageManager()` becomes the single resolution point: + +1. `detect({ programmatic: true, cwd })` — `programmatic: true` disables both the install prompt and the `defaultAgent: 'prompt'` path, so `ni` can only return an agent or `null`. +2. If `detect()` returns `null`, use the CLI's documented default (`yarn`). +3. Validate the agent against `agents`. A non-member throws — it can only come from a version skew in `ni`. +4. `cmdExists(bin)` on the resolved agent. If missing, log the detected agent, the detection source, and the substitution, then walk a deterministic candidate list (`yarn`, then `npm`). If none is available, throw. +5. Build the executable forms once: `command` for `shell: true` call sites, `argv` for `spawn` without a shell. `getVoltaPrefix()` is applied to both and never to `agent`. + +`getPreferredPackageManager()` is kept as a thin wrapper returning `command`, so the change lands without touching all six call sites at once; the two call sites that misuse the value today are corrected to read `agent` and `argv`. + +### Architecture Overview + +```mermaid +flowchart TD + A[resolvePackageManager cwd] --> B["detect({ programmatic: true, cwd })"] + B -->|null| C[default agent: yarn] + B -->|agent| D{agent in agents?} + C --> D + D -->|no| E[throw UnknownAgentError] + D -->|yes| F{cmdExists agent bin?} + F -->|yes| G[build command + argv] + F -->|no| H["log: detected AGENT from SOURCE, not on PATH"] + H --> I[try candidates: yarn, npm] + I -->|none available| J[throw NoAvailablePackageManagerError] + I -->|found| K["log: using CANDIDATE instead"] + K --> G + G --> L["{ agent, command, argv }"] + L --> M["shell: true call sites use command"] + L --> N["spawn without shell uses argv"] + L --> O["comparisons use agent"] +``` + +### Alternatives Considered + +| Alternative | Pros | Cons | Verdict | +|---|---|---|---| +| **ni library API + validation + substitution (chosen)** | Removes the prompt path entirely; closed value set; zero regression; no new dependency; one fewer process spawn | Slightly more code than the current four lines | **Accepted** | +| Keep spawning `na ?`, sanitise stdout | Smallest diff | Treats the symptom. The prompt path stays reachable, and any future `ni` stdout change re-opens the class. `programmatic` cannot be passed to the binary at all | Rejected | +| Fail fast when the agent is not on `PATH` | Cleanest message; stops before `generate`/`cache-graphql` waste time | `cmdExists` is `which.sync`, which can miss a command a shell would still resolve; would convert some working builds into failures — the exact risk this spec is meant to avoid | Rejected as the default; recorded in Decision 3 | +| Pin the agent through an env var or CLI flag | Fully deterministic; pipeline decides once | Adds surface to every caller and to the pipeline; does not fix the unvalidated-string path on its own | Rejected for now; compatible follow-up if WebOps later wants to pin explicitly | +| Align `nextjs.Dockerfile` precedence with `ni` instead of changing the CLI | Fixes the disagreement at its root | Would switch stores like the incident's to install from `pnpm-lock.yaml`, changing their production dependency tree. Unacceptable risk for a diagnostics fix | Rejected | + +### Risks & Mitigations + +| Risk | Impact | Likelihood | Mitigation | +|---|---|---|---| +| `cmdExists` false negative substitutes an agent that was actually usable | Med | Low | Substitution is logged with the detected agent and the reason, so the operator can see and challenge it. Nothing throws on this path — the build proceeds | +| Substituting an agent silently hides a real lockfile inconsistency | Med | Med | The log line names the detection source (`pnpm-lock.yaml`), which is the actionable signal. Store-level linting is listed as out of scope but recommended as follow-up | +| Behaviour drift between the resolved agent and the agent that installed `node_modules` | Med | Med | Pre-existing and unchanged by this spec: the incident's build already installed with yarn and would have built with pnpm. Accepted as an open gap — closing it requires the pipeline-side work listed in Out of Scope | +| `detect({ programmatic: true })` changes the no-lockfile outcome | Low | Med | Explicitly pinned by Decision 4: the CLI's own default (`yarn`) wins over `ni`'s programmatic `npm`, preserving today's documented default | +| Callers still treating the return value as a bare binary name | Med | Low | FR-7 plus the corrections in `start.ts` and `dependencies.ts`; the structured return makes the misuse a type error rather than a runtime surprise | +| 3.x stores do not benefit | High | High | Decision 5: backport to `origin/3.x` by widening the existing fallback guard | + +### Key Decisions + +#### Decision 1: Resolve through `ni`'s library API, not the `na` binary + +- **Status**: Accepted +- **Context**: `programmatic: true` is what disables both the install prompt and the `prompt` default agent. It is an option of `detect()` / `getCliCommand()`; the `na` CLI entry point never exposes it. As long as resolution goes through the binary, the interactive path stays reachable. `@antfu/ni@0.21.12` exports `detect`, `getCommand`, `cmdExists`, `getVoltaPrefix`, `AGENTS`, and `agents` from its package root. +- **Decision**: Call `detect({ programmatic: true, cwd })` in-process and compose the command from `getCommand(agent, 'agent')` plus `getVoltaPrefix()`. Drop the `na` spawn, and with it the `getPackageRootDir` / `getDepPackageJSON` binary-path resolution that exists only to locate it. +- **Consequences**: The prompt path becomes unreachable by construction rather than by validation. One fewer Node process per resolution. `getPackageRootDir`, `getDepPackageJSON` and their private `loadPackageJsonAt` existed only to locate the `na` binary and nothing else in `packages/` imports them, so they are removed. That leaves `resolve-pkg` unused in `@faststore/cli` (`@faststore/diagnostics` declares its own), so it is dropped from the package manifest as well. + +#### Decision 2: Validate the agent against a closed set before it can reach a shell + +- **Status**: Accepted +- **Context**: The defect is not the prompt; it is that any string `ni` prints becomes a shell command. Even with Decision 1, a version skew in `ni` could return something unexpected. +- **Decision**: Check the resolved agent against `ni`'s `agents` list. A non-member throws a named error naming the offending value. Callers never receive an unvalidated string. +- **Consequences**: `command` and `argv` are always derived from a member of a known set. The class of failure this spec addresses cannot recur through a different upstream change. + +#### Decision 3: Unavailable agent → log and substitute, do not fail fast + +- **Status**: Accepted +- **Context**: Fail-fast produces the cleaner message, but `cmdExists` is `which.sync`, which does not necessarily agree with what a shell would resolve. Throwing on a false negative would break a build that works. In the incident specifically, substituting yarn would have been *correct*: the pipeline's `BUILD_COMMAND` was already `yarn build` and `deps` had already installed with yarn — `ni` was the component that disagreed. +- **Decision**: When the resolved agent's binary is absent, log the detected agent, the detection source, and the substitution, then use the first available of `yarn`, `npm`. Throw only when no candidate is available. +- **Consequences**: Regression risk is zero — every path that produced a working command still produces the same one. The operator gets the diagnostic that was previously lost to stderr. The trade-off is that a lockfile inconsistency is reported rather than enforced; enforcement is deliberately left to store-level linting. This decision alone is sufficient for the observed incident: substituting `yarn` matches the manager that installed `node_modules`, so the build completes with no pipeline change required. +- **Alternative recorded**: fail-fast on `!cmdExists`. Cheap to switch later — it is one branch in `resolvePackageManager` — if the fleet turns out to have no false negatives and the team prefers enforcement. + +#### Decision 4: Keep `yarn` as the CLI's default when detection yields nothing + +- **Status**: Accepted +- **Context**: `detect()` returns `null` when there is no lockfile and no `packageManager` field. Today `getPreferredPackageManager` initialises `agent = 'yarn'` and `ni`'s non-programmatic path would reach `defaultAgent: 'prompt'`. Under `programmatic: true`, `getDefaultAgent` would return `npm` instead — a silent change of default. +- **Decision**: On `null`, the CLI uses `yarn`, matching its current documented default. `ni`'s `defaultAgent` config is not consulted. +- **Consequences**: The no-lockfile case stays deterministic and unchanged. The CLI's default is stated in one place instead of being an emergent property of `ni`'s config file. + +#### Decision 5: Backport to 3.x by widening the existing guard, not by porting the new contract + +- **Status**: Accepted +- **Context**: The affected store runs 3.x, and `origin/3.x` is maintained. Its resolution is `spawnSync('na', ['?'], { shell: true })` with `if (agent === '') return 'yarn'`. That guard catches empty stdout but not polluted stdout — which is exactly the incident. Note also that in 3.x this guard means a build with `CI` set in the environment already falls back to yarn and succeeds; removing it would break those. +- **Decision**: On `3.x`, widen the guard from "is empty" to "is not a member of `agents`", keeping the `yarn` fallback and adding the diagnostic log. Do not port the structured return or the library-API rewrite. +- **Consequences**: A strict superset of today's 3.x behaviour — everything that falls back still falls back, and the incident now falls back too instead of producing a shell syntax error. Minimal, reviewable diff on a maintenance branch. The full contract change lands only on `dev`. +- **Why it matters more than it looks**: because every pipeline-side intervention is out of scope, the backport is the *only* thing that reaches stores on the 3.x line. `dev` is at 4.5.0-dev, and `origin/3.x` is actively maintained, which implies stores stay on it deliberately. Without this step the fix covers 4.x stores only. + +#### Decision 6: Split the return value into identity and executable forms + +- **Status**: Accepted +- **Context**: The current return value is overloaded, and two call sites are already wrong because of it. `getVoltaPrefix()` can make it `volta run yarn`, but `start.ts:40` passes it as `argv[0]` to `spawn` without a shell (which would fail with `ENOENT`), and `dependencies.ts:16` compares it with `=== 'npm'` (which would silently pick `add` over `install`). `getPreferredPackageManager` is internal — `packages/cli/src/index.ts` exports only `run` and `commands` — so the shape can change without the public-signature approval the root `AGENTS.md` requires. +- **Decision**: Return `{ agent, command, argv }`. `agent` for comparisons, `command` for `shell: true`, `argv` for `spawn` without a shell. Keep `getPreferredPackageManager()` as a wrapper returning `command` so unaffected call sites are untouched. +- **Consequences**: Two latent bugs are fixed as a by-product. Future call sites are pushed toward the correct form by the type. Diff stays proportional: one new util plus two corrected call sites. + +### Implementation Plan + +1. **Resolution util** — add `resolvePackageManager(cwd?)` to `packages/cli/src/utils/commands.ts` implementing Decisions 1–4 and 6. Remove the `na` spawn. Keep `getPreferredPackageManager()` as a wrapper returning `command`. +2. **Tests** — add `packages/cli/src/utils/commands.test.ts` covering: detection from each lockfile; the two-lockfile precedence that caused the incident; unavailable agent → substitution plus log; no candidate → throw; non-member agent → throw; `null` detection → `yarn`; Volta prefix present in `command`/`argv` and absent from `agent`. Mock `@antfu/ni` and `PATH` lookups per the package's testing convention. +3. **Correct the misusing call sites** — `commands/start.ts` consumes `argv` for the non-shell `spawn` and `command` for the `shell: true` one; `utils/dependencies.ts` compares `agent === 'npm'`. +4. **Verify the untouched call sites** — `build.ts`, `dev.ts`, `test.ts`, `generate-graphql.ts` keep using `getPreferredPackageManager()`; confirm each still receives the shell-appropriate form. +5. **Rebuild and validate downstream** — `pnpm build` in `packages/cli` before running any downstream `generate`, per the non-negotiable build ordering in `packages/cli/AGENTS.md`. +6. **Reproduce the incident locally** — in a fixture with both `pnpm-lock.yaml` and `yarn.lock` and no `pnpm` on `PATH`, confirm `faststore build` logs the substitution and completes instead of emitting `/bin/sh: syntax error`. +7. **Backport** — open a separate PR against `origin/3.x` implementing Decision 5 only. +8. **PR description** — record the `@antfu/ni` API-surface change (binary → library, same pinned version, no new dependency) per the root `AGENTS.md` dependency-discipline checklist, and link the `dk-cicd-hub` counterpart. + +--- + +## 3. Technical Contract + +### Data Models + +```ts +import type { Agent } from '@antfu/ni' + +interface ResolvedPackageManager { + /** Validated member of ni's `agents`. Use for comparisons — never for execution. */ + agent: Agent + /** Executable form for `spawnSync(cmd, { shell: true })`. May include `volta run`. */ + command: string + /** Executable form for `spawn(file, args)` without a shell. */ + argv: [string, ...string[]] +} +``` + +The returned value deliberately carries no detection-source field. Reporting "detected `pnpm` **from `pnpm-lock.yaml`**" would mean reimplementing `ni`'s precedence rules inside the CLI, which would then drift from `ni`. Instead the diagnostic lists the lockfiles actually committed, read from `ni`'s own exported `LOCKS`, without asserting which one won. That is equally actionable and cannot disagree with `ni`. + +Invariant on the three forms, for `agent: 'yarn'`: + +| Volta present | `agent` | `command` | `argv` | +|---|---|---|---| +| no | `'yarn'` | `'yarn'` | `['yarn']` | +| yes | `'yarn'` | `'volta run yarn'` | `['volta','run','yarn']` | + +### Interfaces + +```ts +// packages/cli/src/utils/commands.ts + +/** + * Resolves the package manager for `cwd` using @antfu/ni in programmatic mode. + * + * Never reads stdin and never writes an interactive prompt. The returned + * `agent` is always a member of ni's `agents` list. + * + * @throws UnknownAgentError resolved agent is not a known agent + * @throws NoAvailablePackageManagerError neither the detected agent nor any + * candidate is available on PATH + */ +export async function resolvePackageManager( + cwd?: string +): Promise + +/** + * Backwards-compatible wrapper returning `ResolvedPackageManager.command`. + * Prefer `resolvePackageManager()` in new code. + */ +export async function getPreferredPackageManager(cwd?: string): Promise + +export class UnknownAgentError extends Error {} +export class NoAvailablePackageManagerError extends Error {} +``` + +Candidate order when the detected agent is unavailable: `['yarn', 'npm']`, first one satisfying `cmdExists`. + +Diagnostic emitted on substitution, via `logger.warn`. It must be `warn` and not `log`: `logger.log` is suppressed unless `DISCOVERY_DEBUG=true`, so a `log` call would be invisible in CI, which is where this matters. + +``` +warning - Detected "pnpm" but it is not installed in this environment. Using "yarn" instead. +More than one lockfile is committed (pnpm-lock.yaml, yarn.lock). Keep a single one so +the package manager is unambiguous. +``` + +The second line appears only when more than one lockfile is present in `cwd`. + +### Integration Points + +- **`@antfu/ni@0.21.12`** (existing `dependencies` entry, pinned via `pnpm-workspace.yaml:6` `catalog:`). Consumed API: `detect`, `getCommand`, `cmdExists`, `getVoltaPrefix`, `agents`, and the `Agent` type. The `na` binary is no longer invoked. +- **CLI call sites**: `commands/build.ts:55`, `commands/dev.ts:64`, `commands/start.ts:30`, `commands/test.ts:33`, `commands/generate-graphql.ts:33`, `utils/dependencies.ts:15`. +- **`utils/logger.ts`** — the substitution diagnostic and the `ni`-unresolvable fallback notice. +- **`dk-cicd-hub` → `dk_hub/dockerfiles/nextjs.Dockerfile`** — the `builder` stage does not carry the package manager that `deps` installed, and its lockfile precedence differs from `ni`'s. Tracked separately; this spec makes the CLI resilient to that disagreement but does not resolve it. +- **`origin/3.x`** — receives Decision 5 only. + +### Invariants & Constraints + +- Resolution MUST NOT read stdin, and MUST NOT write an interactive prompt to stdout or stderr, on any path. +- `ResolvedPackageManager.agent` MUST be a member of `ni`'s `agents` list. Any other value MUST throw before the value can be used. +- `agent` MUST NOT contain a Volta prefix; `command` and `argv` MUST contain it whenever `getVoltaPrefix()` is non-empty. +- Every string the CLI interpolates into a `shell: true` command MUST derive from `command`, never from raw `ni` output. +- `spawn` calls without a shell MUST use `argv`, never `command`. +- Any resolution outcome other than a successful one MUST be either logged (substitution) or thrown (unknown agent, no candidate). Silent empty or partial values are FORBIDDEN. +- Diagnostics MUST name only the agent, the detection source, and the substitution. Secrets, tokens, and `.env` values MUST NOT appear. +- Behaviour for repositories whose detected agent is installed MUST be byte-identical to the pre-change behaviour, Volta prefix included. +- The 3.x backport MUST preserve the existing `yarn` fallback; widening the guard is permitted, removing it is FORBIDDEN. From 86f1732025d187d6c24899258642379872b7a8e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1lber=20Laux?= Date: Wed, 29 Jul 2026 16:11:05 -0300 Subject: [PATCH 2/4] fix: resolve the package manager against the target directory Review feedback on #3422. resolvePackageManager() takes a cwd, but both call sites were calling it with no argument, so detection ran against process.cwd() instead of the directory the command operates on. That is wrong whenever the CLI is invoked against another path, which `faststore start ` supports. Pass the store root in start.ts and the install directory in dependencies.ts. `faststore start` also ignored the exit status of the build it triggers, so a failed build was followed by `next start` against a missing .next. It now stops with the reason instead. The unknown-agent test used "deno", which is not in ni 0.21.12's agent list but would become one if the dependency were bumped, silently turning the assertion into a different code path. It now uses a string that cannot ever be a real agent. Ref: FAS-1199 --- packages/cli/src/commands/start.test.ts | 16 ++++++++++++++++ packages/cli/src/commands/start.ts | 10 ++++++++-- packages/cli/src/utils/commands.test.ts | 4 +++- packages/cli/src/utils/dependencies.test.ts | 1 + packages/cli/src/utils/dependencies.ts | 2 +- 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/start.test.ts b/packages/cli/src/commands/start.test.ts index 5e8470a555..a9fa9a62ac 100644 --- a/packages/cli/src/commands/start.test.ts +++ b/packages/cli/src/commands/start.test.ts @@ -39,6 +39,7 @@ describe('Start', () => { storeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'faststore-start-')) fs.mkdirSync(path.join(storeDir, '.next')) vi.clearAllMocks() + spawnSyncMock.mockReturnValue({ status: 0 }) }) afterEach(() => { @@ -54,6 +55,7 @@ describe('Start', () => { await runStart() + expect(resolvePackageManagerMock).toHaveBeenCalledWith(storeDir) expect(spawnMock).toHaveBeenCalledWith( 'yarn', ['next', 'start', path.join(storeDir, '.faststore'), '-p', '3000'], @@ -99,5 +101,19 @@ describe('Start', () => { '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() }) }) diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index 4962503bad..7f71179721 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -27,13 +27,19 @@ export default class Start extends Command { const basePath = getBasePath(args.path) const port = args.port ?? 3000 const { getRoot, tmpDir } = withBasePath(basePath) - const { command, argv } = await resolvePackageManager() + const { command, argv } = await resolvePackageManager(basePath) if (!existsSync(path.join(getRoot(), '.next'))) { - spawnSync(`${command} 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 diff --git a/packages/cli/src/utils/commands.test.ts b/packages/cli/src/utils/commands.test.ts index 445d42fbaf..8f3fd36bf1 100644 --- a/packages/cli/src/utils/commands.test.ts +++ b/packages/cli/src/utils/commands.test.ts @@ -153,7 +153,9 @@ describe('resolvePackageManager', () => { }) it('throws instead of forwarding an unknown agent to a shell', async () => { - vi.mocked(detect).mockResolvedValue('deno' as never) + // 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) }) diff --git a/packages/cli/src/utils/dependencies.test.ts b/packages/cli/src/utils/dependencies.test.ts index 984c140f83..413dba03a4 100644 --- a/packages/cli/src/utils/dependencies.test.ts +++ b/packages/cli/src/utils/dependencies.test.ts @@ -35,6 +35,7 @@ describe('installDependencies', () => { await install() + expect(resolvePackageManagerMock).toHaveBeenCalledWith('/store') expect(runCommandSyncMock).toHaveBeenCalledWith( expect.objectContaining({ cmd: 'yarn add preact@10.23.1' }) ) diff --git a/packages/cli/src/utils/dependencies.ts b/packages/cli/src/utils/dependencies.ts index 03a4caabcc..0178a2472c 100644 --- a/packages/cli/src/utils/dependencies.ts +++ b/packages/cli/src/utils/dependencies.ts @@ -12,7 +12,7 @@ export async function installDependencies({ cwd, errorMessage, }: InstallDependenciesOptions) { - const { agent, command } = await resolvePackageManager() + const { agent, command } = await resolvePackageManager(cwd) const installCommand = agent === 'npm' ? 'install' : 'add' runCommandSync({ From 1a06a13dd7feafab89f4792831a863c786c7ded8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1lber=20Laux?= Date: Thu, 30 Jul 2026 18:38:08 -0300 Subject: [PATCH 3/4] fix: refuse package manager substitution when installing dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #3422. installDependencies is the one call site that mutates the project: substituting e.g. yarn into a pnpm store would run `yarn add`, writing a yarn.lock next to pnpm-lock.yaml — manufacturing exactly the dual-lockfile ambiguity behind FAS-1199 (and yarn may not understand workspace: ranges in a pnpm project). resolvePackageManager() now takes { substitute }, and the install path resolves with substitute: false, throwing an actionable error instead of writing a conflicting lockfile. The missing-dependencies loop in generate.ts was forEach(async ...), so that throw would surface as an unhandled rejection instead of going through oclif's error handling. It is now a for...of, with the spinner stopped in a finally. FALLBACK_AGENTS gains pnpm and bun as last resorts: the branch where yarn and npm are both missing previously threw even when a usable package manager was installed. Substitution order is unchanged for every case that resolved before. Ref: FAS-1199 Co-Authored-By: Claude Fable 5 --- packages/cli/src/utils/commands.test.ts | 27 +++++++++++++++++ packages/cli/src/utils/commands.ts | 32 +++++++++++++++++---- packages/cli/src/utils/dependencies.test.ts | 4 ++- packages/cli/src/utils/dependencies.ts | 7 ++++- packages/cli/src/utils/generate.ts | 22 ++++++++------ specs/package-manager-resolution.md | 31 +++++++++++++------- 6 files changed, 97 insertions(+), 26 deletions(-) diff --git a/packages/cli/src/utils/commands.test.ts b/packages/cli/src/utils/commands.test.ts index 8f3fd36bf1..86847893fc 100644 --- a/packages/cli/src/utils/commands.test.ts +++ b/packages/cli/src/utils/commands.test.ts @@ -118,6 +118,33 @@ describe('resolvePackageManager', () => { 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'), '') diff --git a/packages/cli/src/utils/commands.ts b/packages/cli/src/utils/commands.ts index 2d0f75c730..cf730b4d31 100644 --- a/packages/cli/src/utils/commands.ts +++ b/packages/cli/src/utils/commands.ts @@ -25,8 +25,21 @@ export class UnknownAgentError extends Error {} export class NoAvailablePackageManagerError extends Error {} +export interface ResolvePackageManagerOptions { + /** + * Whether a detected-but-not-installed agent may be substituted with an + * available fallback. Callers that write to the project (dependency + * installation) must pass `false`: installing with a substitute agent would + * leave a second, conflicting lockfile next to the project's own. + */ + substitute?: boolean +} + const DEFAULT_AGENT: Agent = 'yarn' -const FALLBACK_AGENTS: Agent[] = ['yarn', 'npm'] +// yarn and npm first — they are what the store build images ship. The +// remaining known agents are last resorts so an environment that only has +// pnpm or bun still resolves instead of failing. +const FALLBACK_AGENTS: Agent[] = ['yarn', 'npm', 'pnpm', 'bun'] /** * Resolves the package manager to use for `cwd`. @@ -36,7 +49,8 @@ const FALLBACK_AGENTS: Agent[] = ['yarn', 'npm'] * callers interpolate this value straight into shell commands. */ export async function resolvePackageManager( - cwd: string = process.cwd() + cwd: string = process.cwd(), + { substitute = true }: ResolvePackageManagerOptions = {} ): Promise { const detected = (await detect({ programmatic: true, cwd })) ?? DEFAULT_AGENT @@ -48,9 +62,17 @@ export async function resolvePackageManager( ) } - const agent = cmdExists(binOf(detected)) - ? detected - : substituteAgent(detected, cwd) + let agent = detected + + if (!cmdExists(binOf(detected))) { + if (!substitute) { + throw new NoAvailablePackageManagerError( + `Detected "${detected}", which is not installed in this environment. This operation writes a lockfile, so no other package manager can stand in for it. Install "${detected}", or remove the lockfile or "packageManager" field that selects it.` + ) + } + + agent = substituteAgent(detected, cwd) + } const voltaPrefix = getVoltaPrefix() const command = voltaPrefix ? `${voltaPrefix} ${binOf(agent)}` : binOf(agent) diff --git a/packages/cli/src/utils/dependencies.test.ts b/packages/cli/src/utils/dependencies.test.ts index 413dba03a4..ef860e5b98 100644 --- a/packages/cli/src/utils/dependencies.test.ts +++ b/packages/cli/src/utils/dependencies.test.ts @@ -35,7 +35,9 @@ describe('installDependencies', () => { await install() - expect(resolvePackageManagerMock).toHaveBeenCalledWith('/store') + expect(resolvePackageManagerMock).toHaveBeenCalledWith('/store', { + substitute: false, + }) expect(runCommandSyncMock).toHaveBeenCalledWith( expect.objectContaining({ cmd: 'yarn add preact@10.23.1' }) ) diff --git a/packages/cli/src/utils/dependencies.ts b/packages/cli/src/utils/dependencies.ts index 0178a2472c..7aca7149f9 100644 --- a/packages/cli/src/utils/dependencies.ts +++ b/packages/cli/src/utils/dependencies.ts @@ -12,7 +12,12 @@ export async function installDependencies({ cwd, errorMessage, }: InstallDependenciesOptions) { - const { agent, command } = await resolvePackageManager(cwd) + // Installing writes a lockfile: a substitute agent would leave a second, + // conflicting one next to the project's (e.g. `yarn add` in a pnpm store + // creates a `yarn.lock`), so only the detected agent is acceptable here. + const { agent, command } = await resolvePackageManager(cwd, { + substitute: false, + }) const installCommand = agent === 'npm' ? 'install' : 'add' runCommandSync({ diff --git a/packages/cli/src/utils/generate.ts b/packages/cli/src/utils/generate.ts index f2534edd07..38d1d1e4d4 100644 --- a/packages/cli/src/utils/generate.ts +++ b/packages/cli/src/utils/generate.ts @@ -582,7 +582,9 @@ async function validateAndInstallMissingDependencies(basePath: string) { }) } - missingDependencies.forEach(async ({ feature, dependencies }) => { + // Not `forEach(async …)`: a rejection inside a floating callback is an + // unhandled rejection, which crashes past oclif's error handling. + for (const { feature, dependencies } of missingDependencies) { const dependenciesToInstall = dependencies.filter((dependency) => { const dependencyName = dependency.split('@')[0] return !userPackageJson.dependencies[dependencyName] @@ -593,15 +595,17 @@ async function validateAndInstallMissingDependencies(basePath: string) { `Installing ${feature} missing dependencies\n` ).start() - await installDependencies({ - dependencies: dependenciesToInstall, - cwd: userDir, - errorMessage: `failed to install ${feature} dependencies`, - }) - - spinner.stop() + try { + await installDependencies({ + dependencies: dependenciesToInstall, + cwd: userDir, + errorMessage: `failed to install ${feature} dependencies`, + }) + } finally { + spinner.stop() + } } - }) + } } async function enableSearchSSR(basePath: string) { diff --git a/specs/package-manager-resolution.md b/specs/package-manager-resolution.md index 61eee2bb17..4e364c1b81 100644 --- a/specs/package-manager-resolution.md +++ b/specs/package-manager-resolution.md @@ -89,7 +89,7 @@ Make package-manager resolution **safe by construction** and **self-explanatory | 1 | Happy path | `yarn.lock` only; `yarn` on `PATH` | `faststore build` | Resolves `yarn`; runs `yarn run build` in `.faststore`; no new log lines | | 2 | Happy path (Volta) | `yarn.lock` only; `volta` and `yarn` on `PATH` | `faststore build` | Shell command is `volta run yarn run build`; `spawn` without shell uses argv `['volta','run','yarn']` | | 3 | Error → the incident | `pnpm-lock.yaml` + `yarn.lock`; only `yarn` on `PATH` | `faststore build` | Logs that `pnpm` was detected from `pnpm-lock.yaml` and is not installed, and that `yarn` is used instead; build proceeds; no shell syntax error | -| 4 | Error | `pnpm-lock.yaml` only; neither `pnpm` nor `yarn` on `PATH`; `npm` present | `faststore build` | Logs the detection and the substitution; resolves `npm`; `installDependencies` uses `npm install`, not `npm add` | +| 4 | Error | `pnpm-lock.yaml` only; neither `pnpm` nor `yarn` on `PATH`; `npm` present | `faststore build` | Logs the detection and the substitution; the build resolves and runs with `npm`. If the generate step needs to install missing feature dependencies, it throws the named error instead of writing a `package-lock.json` next to `pnpm-lock.yaml` | | 5 | Error | No usable agent on `PATH` at all | `faststore build` | Throws a named error listing the detected agent and the candidates tried; never emits a partial shell command | | 6 | Edge | No lockfile, no `packageManager` field | `faststore build` | Resolves the documented default (`yarn` when available) deterministically; `ni`'s `defaultAgent: 'prompt'` never surfaces | | 7 | Edge | `package.json` has `packageManager` naming an agent `ni` does not know | `faststore build` | Falls through to lockfile detection as `ni` already does; the unknown value is reported once, and never used as a command | @@ -99,7 +99,7 @@ Make package-manager resolution **safe by construction** and **self-explanatory - **FR-1**: Resolution MUST use `@antfu/ni`'s library API with `programmatic: true`, not the `na` binary. The binary exposes no way to disable the interactive fallback. - **FR-2**: Resolution MUST validate the resolved agent against `ni`'s `agents` list before the value is used to build any command. -- **FR-3**: When the resolved agent's binary is not on `PATH`, resolution MUST log an explicit diagnostic and substitute the first available candidate, rather than failing. +- **FR-3**: When the resolved agent's binary is not on `PATH`, resolution MUST log an explicit diagnostic and substitute the first available candidate, rather than failing. Substitution is reserved for call sites that only *run* commands: a caller whose operation writes to the project (dependency installation) MUST resolve with substitution disabled, and resolution MUST then throw instead — installing with a substitute agent would write a second, conflicting lockfile. - **FR-4**: When no candidate is available, resolution MUST throw a named error. It MUST NOT return a partially-formed or empty command. - **FR-5**: Resolution MUST return the agent identity and the executable form as separate values, so callers stop pattern-matching a command string. - **FR-6**: The `volta run` prefix MUST be preserved in the executable form, and MUST NOT leak into the agent identity. @@ -153,8 +153,10 @@ flowchart TD D -->|no| E[throw UnknownAgentError] D -->|yes| F{cmdExists agent bin?} F -->|yes| G[build command + argv] - F -->|no| H["log: detected AGENT from SOURCE, not on PATH"] - H --> I[try candidates: yarn, npm] + F -->|no| P{substitution allowed?} + P -->|"no — mutating caller"| Q[throw NoAvailablePackageManagerError] + P -->|yes| H["log: detected AGENT from SOURCE, not on PATH"] + H --> I[try candidates: yarn, npm, pnpm, bun] I -->|none available| J[throw NoAvailablePackageManagerError] I -->|found| K["log: using CANDIDATE instead"] K --> G @@ -205,9 +207,10 @@ flowchart TD - **Status**: Accepted - **Context**: Fail-fast produces the cleaner message, but `cmdExists` is `which.sync`, which does not necessarily agree with what a shell would resolve. Throwing on a false negative would break a build that works. In the incident specifically, substituting yarn would have been *correct*: the pipeline's `BUILD_COMMAND` was already `yarn build` and `deps` had already installed with yarn — `ni` was the component that disagreed. -- **Decision**: When the resolved agent's binary is absent, log the detected agent, the detection source, and the substitution, then use the first available of `yarn`, `npm`. Throw only when no candidate is available. -- **Consequences**: Regression risk is zero — every path that produced a working command still produces the same one. The operator gets the diagnostic that was previously lost to stderr. The trade-off is that a lockfile inconsistency is reported rather than enforced; enforcement is deliberately left to store-level linting. This decision alone is sufficient for the observed incident: substituting `yarn` matches the manager that installed `node_modules`, so the build completes with no pipeline change required. -- **Alternative recorded**: fail-fast on `!cmdExists`. Cheap to switch later — it is one branch in `resolvePackageManager` — if the fleet turns out to have no false negatives and the team prefers enforcement. +- **Decision**: When the resolved agent's binary is absent, log the detected agent, the detection source, and the substitution, then use the first available of `yarn`, `npm`, and then the remaining known agents (`pnpm`, `bun`) as last resorts. Throw only when no candidate is available. +- **Scope (review follow-up)**: Substitution applies only to call sites that *run* the project (`build`, `dev`, `start`, `test`, `generate-graphql`). The one mutating call site, `installDependencies`, resolves with `substitute: false` and gets a thrown error instead: running e.g. `yarn add` in a pnpm store writes a `yarn.lock` next to `pnpm-lock.yaml`, manufacturing exactly the dual-lockfile ambiguity behind the incident (and `yarn` may not understand `workspace:` ranges in a pnpm project). A deliberate, explained failure is preferable to silently corrupting the store's lockfile state. +- **Consequences**: Regression risk is zero — every path that produced a working command still produces the same one, and every path that now throws was already failing before this spec (with an unattributable shell error). The operator gets the diagnostic that was previously lost to stderr. The trade-off is that a lockfile inconsistency is reported rather than enforced on the run path; enforcement is deliberately left to store-level linting. This decision alone is sufficient for the observed incident: substituting `yarn` matches the manager that installed `node_modules`, so the build completes with no pipeline change required. +- **Alternative recorded**: fail-fast on `!cmdExists` everywhere. Cheap to switch later — it is one branch in `resolvePackageManager` — if the fleet turns out to have no false negatives and the team prefers enforcement. #### Decision 4: Keep `yarn` as the CLI's default when detection yields nothing @@ -281,12 +284,19 @@ Invariant on the three forms, for `agent: 'yarn'`: * Never reads stdin and never writes an interactive prompt. The returned * `agent` is always a member of ni's `agents` list. * + * `substitute: false` is for callers that write to the project: with it, a + * detected-but-missing agent throws instead of being substituted, because + * installing with a substitute would write a conflicting lockfile. + * * @throws UnknownAgentError resolved agent is not a known agent * @throws NoAvailablePackageManagerError neither the detected agent nor any - * candidate is available on PATH + * candidate is available on PATH, or + * the detected agent is missing and + * `substitute` is `false` */ export async function resolvePackageManager( - cwd?: string + cwd?: string, + options?: { substitute?: boolean } ): Promise /** @@ -299,7 +309,7 @@ export class UnknownAgentError extends Error {} export class NoAvailablePackageManagerError extends Error {} ``` -Candidate order when the detected agent is unavailable: `['yarn', 'npm']`, first one satisfying `cmdExists`. +Candidate order when the detected agent is unavailable: `['yarn', 'npm', 'pnpm', 'bun']`, first one satisfying `cmdExists`. `yarn` and `npm` lead because they are what the store build images ship; the remaining known agents are last resorts so an environment that only has one of them still resolves. Diagnostic emitted on substitution, via `logger.warn`. It must be `warn` and not `log`: `logger.log` is suppressed unless `DISCOVERY_DEBUG=true`, so a `log` call would be invisible in CI, which is where this matters. @@ -327,6 +337,7 @@ The second line appears only when more than one lockfile is present in `cwd`. - Every string the CLI interpolates into a `shell: true` command MUST derive from `command`, never from raw `ni` output. - `spawn` calls without a shell MUST use `argv`, never `command`. - Any resolution outcome other than a successful one MUST be either logged (substitution) or thrown (unknown agent, no candidate). Silent empty or partial values are FORBIDDEN. +- Call sites that write to the project (dependency installation) MUST resolve with `substitute: false`; substitution is reserved for call sites that only run commands. - Diagnostics MUST name only the agent, the detection source, and the substitution. Secrets, tokens, and `.env` values MUST NOT appear. - Behaviour for repositories whose detected agent is installed MUST be byte-identical to the pre-change behaviour, Volta prefix included. - The 3.x backport MUST preserve the existing `yarn` fallback; widening the guard is permitted, removing it is FORBIDDEN. From aedcdcf1dd1aae7451e50692f7c5fca1ffcff934 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1lber=20Laux?= Date: Thu, 30 Jul 2026 23:41:35 -0300 Subject: [PATCH 4/4] refactor: build argv as a tuple instead of asserting over a split Review feedback on #3422 (CodeRabbit). Constructing argv as a non-empty tuple and deriving command from it removes the type assertion on command.split(' '). Same values, one source of truth for both forms. Also asserts in start.test.ts that the build step is skipped when .next already exists, making the no-build branch explicit. Ref: FAS-1199 Co-Authored-By: Claude Fable 5 --- packages/cli/src/commands/start.test.ts | 1 + packages/cli/src/utils/commands.ts | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/start.test.ts b/packages/cli/src/commands/start.test.ts index a9fa9a62ac..e4548c87ec 100644 --- a/packages/cli/src/commands/start.test.ts +++ b/packages/cli/src/commands/start.test.ts @@ -61,6 +61,7 @@ describe('Start', () => { ['next', 'start', path.join(storeDir, '.faststore'), '-p', '3000'], { stdio: 'inherit' } ) + expect(spawnSyncMock).not.toHaveBeenCalled() }) it('splits a Volta prefix into the spawn arguments', async () => { diff --git a/packages/cli/src/utils/commands.ts b/packages/cli/src/utils/commands.ts index cf730b4d31..295b1ce536 100644 --- a/packages/cli/src/utils/commands.ts +++ b/packages/cli/src/utils/commands.ts @@ -74,10 +74,14 @@ export async function resolvePackageManager( agent = substituteAgent(detected, cwd) } + const argv: [string, ...string[]] = [binOf(agent)] const voltaPrefix = getVoltaPrefix() - const command = voltaPrefix ? `${voltaPrefix} ${binOf(agent)}` : binOf(agent) - return { agent, command, argv: command.split(' ') as [string, ...string[]] } + if (voltaPrefix) { + argv.unshift(...voltaPrefix.split(' ')) + } + + return { agent, command: argv.join(' '), argv } } /** Shell-ready form of {@link resolvePackageManager}, for callers that only run a command. */