diff --git a/__tests__/dev-package-manager.test.ts b/__tests__/dev-package-manager.test.ts new file mode 100644 index 0000000..f8450e6 --- /dev/null +++ b/__tests__/dev-package-manager.test.ts @@ -0,0 +1,171 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +import { pickPackageManager } from '../src/lib/dev-package-manager'; + +function makeDir(): string { + return mkdtempSync(join(tmpdir(), 'lt-pm-')); +} + +function touch(dir: string, name: string): void { + writeFileSync(join(dir, name), ''); +} + +describe('pickPackageManager', () => { + let dir: string; + + beforeEach(() => { + dir = makeDir(); + }); + + afterEach(() => { + rmSync(dir, { force: true, recursive: true }); + }); + + describe('lockfile detection', () => { + it('picks pnpm when pnpm-lock.yaml is present', () => { + touch(dir, 'pnpm-lock.yaml'); + const pm = pickPackageManager(dir, {}); + expect(pm.bin).toBe('pnpm'); + expect(pm.name).toBe('pnpm'); + }); + + it('picks yarn when yarn.lock is present', () => { + touch(dir, 'yarn.lock'); + const pm = pickPackageManager(dir, {}); + expect(pm.bin).toBe('yarn'); + expect(pm.name).toBe('yarn'); + }); + + it('picks npm when package-lock.json is present', () => { + touch(dir, 'package-lock.json'); + const pm = pickPackageManager(dir, {}); + expect(pm.bin).toBe('npm'); + expect(pm.name).toBe('npm'); + }); + + it('prefers pnpm-lock.yaml over yarn.lock when both exist', () => { + // Two lockfiles is a project state error, but `lt dev` must not + // explode on it — pnpm wins by historical precedent (the CLI + // shipped pnpm-first for years). + touch(dir, 'pnpm-lock.yaml'); + touch(dir, 'yarn.lock'); + const pm = pickPackageManager(dir, {}); + expect(pm.bin).toBe('pnpm'); + }); + + it('prefers pnpm-lock.yaml over package-lock.json when both exist', () => { + touch(dir, 'pnpm-lock.yaml'); + touch(dir, 'package-lock.json'); + const pm = pickPackageManager(dir, {}); + expect(pm.bin).toBe('pnpm'); + }); + + it('prefers yarn.lock over package-lock.json when both exist', () => { + touch(dir, 'yarn.lock'); + touch(dir, 'package-lock.json'); + const pm = pickPackageManager(dir, {}); + expect(pm.bin).toBe('yarn'); + }); + + it('falls back to pnpm when no lockfile is present', () => { + // Historical default — fresh scaffolds and vendored monorepos + // without a lockfile must keep working exactly as before. + const pm = pickPackageManager(dir, {}); + expect(pm.bin).toBe('pnpm'); + expect(pm.name).toBe('pnpm'); + }); + }); + + describe('env overrides', () => { + it('LT_PM_BIN overrides every lockfile signal', () => { + // CI pipeline pin: even with a pnpm-lock checked in, the + // operator can force a different manager (e.g. corepack-managed + // yarn). Wins over the legacy LT_PNPM_BIN too. + touch(dir, 'pnpm-lock.yaml'); + const pm = pickPackageManager(dir, { LT_PM_BIN: '/opt/yarn/bin/yarn', LT_PNPM_BIN: 'pnpm' }); + expect(pm.bin).toBe('/opt/yarn/bin/yarn'); + expect(pm.name).toBe('yarn'); + }); + + it('LT_PNPM_BIN still works as a legacy alias for backwards compatibility', () => { + // Older CI configs set LT_PNPM_BIN long before LT_PM_BIN existed. + // Removing it would silently regress those pipelines, so we keep + // it as a lower-precedence override. + const pm = pickPackageManager(dir, { LT_PNPM_BIN: '/usr/local/bin/pnpm' }); + expect(pm.bin).toBe('/usr/local/bin/pnpm'); + expect(pm.name).toBe('pnpm'); + }); + + it('infers name=unknown for a bin path that does not match a known manager', () => { + // Custom corporate wrapper script — we still spawn it but don't + // guess its identity for log lines. + const pm = pickPackageManager(dir, { LT_PM_BIN: '/opt/internal/wrap-deps' }); + expect(pm.bin).toBe('/opt/internal/wrap-deps'); + expect(pm.name).toBe('unknown'); + }); + }); + + describe('command synthesis', () => { + it('runScript prefixes with `run` so npm is happy', () => { + // `npm dev` is not a reserved alias and fails; `npm run dev` + // works everywhere. We standardise on `run` so call sites can + // emit one arg array regardless of which manager is selected. + touch(dir, 'package-lock.json'); + const pm = pickPackageManager(dir, {}); + expect(pm.runScript('dev')).toEqual(['run', 'dev']); + expect(pm.runScript('test:e2e', ['--shard=1/2'])).toEqual(['run', 'test:e2e', '--shard=1/2']); + }); + + it('installArgs is portable across managers', () => { + // ` install` is the one verb that works identically on all + // three managers — no `add`, no `i` shortcut. + const pm = pickPackageManager(dir, {}); + expect(pm.installArgs).toEqual(['install']); + }); + + it('exec inserts `--` for npm so option flags reach the binary', () => { + // The bug that motivates this: `npm exec playwright test + // --shard=1/2` makes npm treat `--shard` as ITS own flag (and + // crash). The `--` separator forces npm to hand everything after + // it to playwright unchanged. pnpm/yarn route through verbatim + // and don't need the separator. + touch(dir, 'package-lock.json'); + const npm = pickPackageManager(dir, {}); + expect(npm.exec('playwright', ['test', '--shard=1/2'])).toEqual([ + 'exec', + '--', + 'playwright', + 'test', + '--shard=1/2', + ]); + + rmSync(join(dir, 'package-lock.json')); + touch(dir, 'pnpm-lock.yaml'); + const pnpm = pickPackageManager(dir, {}); + expect(pnpm.exec('playwright', ['test', '--shard=1/2'])).toEqual([ + 'exec', + 'playwright', + 'test', + '--shard=1/2', + ]); + }); + }); + + describe('per-component detection in a monorepo', () => { + it('returns different managers for sibling api/app dirs', () => { + // The monorepo case the bug report was filed for: a project with + // an npm api and an pnpm app must drive each component with the + // correct manager. + const apiDir = join(dir, 'api'); + const appDir = join(dir, 'app'); + mkdirSync(apiDir); + mkdirSync(appDir); + touch(apiDir, 'package-lock.json'); + touch(appDir, 'pnpm-lock.yaml'); + expect(pickPackageManager(apiDir, {}).bin).toBe('npm'); + expect(pickPackageManager(appDir, {}).bin).toBe('pnpm'); + }); + }); +}); diff --git a/src/commands/dev/test.ts b/src/commands/dev/test.ts index 5b800a1..932df19 100644 --- a/src/commands/dev/test.ts +++ b/src/commands/dev/test.ts @@ -4,6 +4,7 @@ import { GluegunCommand } from 'gluegun'; import { ExtendedGluegunToolbox } from '../../interfaces/extended-gluegun-toolbox'; import { caddyAvailable, caddyDaemonRunning } from '../../lib/caddy'; import { envBridgePath } from '../../lib/dev-env-bridge'; +import { pickPackageManager } from '../../lib/dev-package-manager'; import { runChildInherit } from '../../lib/dev-process'; import { appNeedsPortPatch, resolveLayout } from '../../lib/dev-project'; import { @@ -78,7 +79,6 @@ const TestCommand: GluegunCommand = { const keep = Boolean(parameters.options.keep) || parameters.options.teardown === false; const debug = Boolean(parameters.options.debug); const forwarded = parameters.array || []; - const pnpmBin = process.env.LT_PNPM_BIN || 'pnpm'; // `--shard N` → run the suite split across N fully-isolated stacks in // parallel. A bare `--shard` defaults to 2 — the stable sweet spot for a // heavy built-SSR suite (N>=3 over-subscribes the perf cores → flaky; see @@ -102,7 +102,8 @@ const TestCommand: GluegunCommand = { return 'dev test: no api'; } info(colors.bold(`Running API tests for "${identity.slug}" (isolated DB)`)); - const code = await runChildInherit(pnpmBin, ['run', 'test:e2e', ...forwarded], { + const apiPm = pickPackageManager(layout.apiDir); + const code = await runChildInherit(apiPm.bin, apiPm.runScript('test:e2e', forwarded), { cwd: layout.apiDir, env: process.env, }); @@ -170,10 +171,11 @@ const TestCommand: GluegunCommand = { try { info(''); info(colors.bold(`Running isolated Playwright E2E for "${identity.slug}" sharded across ${shardTotal} stacks`)); + const shardPm = pickPackageManager(layout.appDir); shardExit = await runShardedTestSession(layout, identity, log, { devDbName, forwarded, - pnpmBin, + pm: shardPm, total: shardTotal, }); } catch (e) { @@ -231,7 +233,11 @@ const TestCommand: GluegunCommand = { info(colors.dim(` app: ${ctx.appUrl} db: ${ctx.dbName}`)); info(''); - exitCode = await runChildInherit(pnpmBin, ['run', 'test:e2e', ...forwarded], { cwd: layout.appDir, env }); + const appPm = pickPackageManager(layout.appDir); + exitCode = await runChildInherit(appPm.bin, appPm.runScript('test:e2e', forwarded), { + cwd: layout.appDir, + env, + }); } catch (e) { error(`Failed to run isolated E2E: ${(e as Error).message}`); exitCode = 1; diff --git a/src/commands/dev/up.ts b/src/commands/dev/up.ts index ade40d8..833cc27 100644 --- a/src/commands/dev/up.ts +++ b/src/commands/dev/up.ts @@ -6,6 +6,7 @@ import { ExtendedGluegunToolbox } from '../../interfaces/extended-gluegun-toolbo import { caddyAvailable, caddyDaemonRunning, CaddyRoute, reloadCaddy, upsertProjectBlock } from '../../lib/caddy'; import { buildDevEnv } from '../../lib/dev-env'; import { writeEnvBridge } from '../../lib/dev-env-bridge'; +import { pickPackageManager } from '../../lib/dev-package-manager'; import { addToGitignore, autoPatch, patchClaudeMd } from '../../lib/dev-patches'; import { killProcessGroup, listenSnapshot, spawnDetached, terminateProcessGroup } from '../../lib/dev-process'; import { resolveLayout } from '../../lib/dev-project'; @@ -345,7 +346,6 @@ const UpCommand: GluegunCommand = { identity, }); - const pnpmBin = process.env.LT_PNPM_BIN || 'pnpm'; const pids: { api?: number; app?: number } = {}; const rotationNotes: string[] = []; const started: string[] = []; @@ -368,7 +368,12 @@ const UpCommand: GluegunCommand = { kept.push('api'); } else { await reclaimPort(existingSession?.pids.api, apiPort, apiHealth ?? 'dead'); - const apiResult = spawnDetached(pnpmBin, ['start'], { + // Per-component PM detection: a monorepo may have an npm api and a + // pnpm app, and the legacy hard-coded `pnpm start` would silently + // regenerate a foreign lockfile + crash on un-approved build + // scripts when run against an npm-only project. + const apiPm = pickPackageManager(layout.apiDir); + const apiResult = spawnDetached(apiPm.bin, apiPm.runScript('start'), { cwd: layout.apiDir, env: devEnv.api.env, logFile: join(layout.root, '.lt-dev', 'api.log'), @@ -390,7 +395,8 @@ const UpCommand: GluegunCommand = { kept.push('app'); } else { await reclaimPort(existingSession?.pids.app, appPort, appHealth ?? 'dead'); - const appResult = spawnDetached(pnpmBin, ['dev'], { + const appPm = pickPackageManager(layout.appDir); + const appResult = spawnDetached(appPm.bin, appPm.runScript('dev'), { cwd: layout.appDir, env: devEnv.app.env, logFile: join(layout.root, '.lt-dev', 'app.log'), diff --git a/src/commands/ticket/start.ts b/src/commands/ticket/start.ts index 09c9922..c880dc5 100644 --- a/src/commands/ticket/start.ts +++ b/src/commands/ticket/start.ts @@ -11,7 +11,7 @@ import { deriveTicketId, gitFetch, gitMainRepoRoot, - pnpmInstall, + installWorktreeDeps, worktreeAdd, worktreePathFor, writeTicketMarker, @@ -105,13 +105,15 @@ const StartCommand: GluegunCommand = { // 2. tag the worktree with its ticket id (makes lt dev * ticket-aware). writeTicketMarker(worktreePath, id); - // 3. install deps (pnpm hard-links from the shared store → fast). + // 3. install deps. Auto-detects pnpm/yarn/npm from the worktree's + // lockfile so an npm-only repo doesn't get a foreign pnpm-lock + // injected on every ticket start. if (parameters.options.install !== false) { - info(colors.dim('Installing dependencies (pnpm) …')); + info(colors.dim('Installing dependencies …')); try { - pnpmInstall(worktreePath); + installWorktreeDeps(worktreePath); } catch (e) { - warning(`pnpm install failed (${(e as Error).message}) — continuing; run it manually in the worktree.`); + warning(`install failed (${(e as Error).message}) — continuing; run it manually in the worktree.`); } } diff --git a/src/lib/dev-package-manager.ts b/src/lib/dev-package-manager.ts new file mode 100644 index 0000000..9ab6e54 --- /dev/null +++ b/src/lib/dev-package-manager.ts @@ -0,0 +1,118 @@ +/** + * Pick the package manager `lt dev` should drive for a given project dir. + * + * `lt dev up` and the related test/ticket flows used to hard-code `pnpm` + * for every monorepo. That breaks npm-only and yarn-only projects: a + * `pnpm install` invoked from inside `pnpm start` regenerates a + * pnpm-lock.yaml (which the project's .gitignore then refuses to track), + * fails on un-approved build scripts (bcrypt, sharp, esbuild) and exits + * non-zero — the supervised api/app processes die immediately and + * `lt dev status` reports them as `dead`. + * + * Detection order — highest precedence first: + * 1. `LT_PM_BIN` env var (generic override). + * 2. `LT_PNPM_BIN` env var (legacy — kept for backwards compatibility). + * 3. `pnpm-lock.yaml` in `cwd` → pnpm + * 4. `yarn.lock` in `cwd` → yarn + * 5. `package-lock.json` in `cwd` → npm + * 6. Fallback: `pnpm` (preserves the historical default so nothing + * breaks for the projects this CLI was originally written for). + * + * The detection is per-cwd so a monorepo with a pnpm api + npm app gets + * the correct command per component. + */ +import { existsSync } from 'node:fs'; +import { join } from 'node:path'; + +export type PackageManager = 'npm' | 'pnpm' | 'yarn'; + +export interface PackageManagerCommand { + /** Binary to spawn (just the name; resolved via PATH). */ + bin: string; + /** Argv to invoke ` install` portably. */ + installArgs: string[]; + /** The detected manager — useful for log lines. */ + name: PackageManager | 'unknown'; + /** + * Argv to execute a project-local binary portably. `exec('playwright', + * ['test'])` → `['exec', 'playwright', 'test']` for pnpm/yarn and + * `['exec', '--', 'playwright', 'test']` for npm. The `--` separator + * is required on npm because npm-exec treats flags after the binary + * as its own; pnpm/yarn pass them through unchanged. Used in place + * of `pnpm exec` for tools like `playwright test` so the resulting + * argv survives the shard-flag forwarding (CI-parity). + */ + exec(binary: string, args?: readonly string[]): string[]; + /** + * Argv to run a package.json script portably. `runScript('dev')` → + * `['run', 'dev']` for npm/yarn, `['dev']` for pnpm. Always uses + * `run` for npm because `npm dev` is NOT a reserved alias and would + * fail; `npm start`/`npm test` happen to work without `run` but using + * `run` everywhere keeps the call sites uniform. + */ + runScript(script: string, extra?: readonly string[]): string[]; +} + +/** + * Resolve which package manager to drive for `cwd`. Pure — only filesystem + * existence checks, no exec. The override env vars take precedence so a + * CI pipeline can pin the manager without touching the lockfile. + * + * `cwd` is expected to be a project root (i.e. where the lockfile lives). + * For a monorepo with separate api/app dirs, call this once per dir. + */ +export function pickPackageManager(cwd: string, env: NodeJS.ProcessEnv = process.env): PackageManagerCommand { + const override = env.LT_PM_BIN || env.LT_PNPM_BIN; + if (override) { + return buildCommand(override, inferNameFromBin(override)); + } + if (existsSync(join(cwd, 'pnpm-lock.yaml'))) { + return buildCommand('pnpm', 'pnpm'); + } + if (existsSync(join(cwd, 'yarn.lock'))) { + return buildCommand('yarn', 'yarn'); + } + if (existsSync(join(cwd, 'package-lock.json'))) { + return buildCommand('npm', 'npm'); + } + // Historical default — keep so projects without a lockfile (fresh + // scaffolds, vendored monorepos) behave exactly as before. + return buildCommand('pnpm', 'pnpm'); +} + +function buildCommand(bin: string, name: PackageManager | 'unknown'): PackageManagerCommand { + const isNpm = name === 'npm'; + return { + bin, + exec(binary: string, args: readonly string[] = []): string[] { + // `npm exec` consumes the args before the binary name and treats + // anything after as its own flags unless separated by `--`. pnpm + // and yarn route them through unchanged. Without the separator + // `npm exec playwright test --shard=1/2` would parse `--shard` + // as an npm option, NOT a Playwright one. + return isNpm ? ['exec', '--', binary, ...args] : ['exec', binary, ...args]; + }, + installArgs: ['install'], + name, + runScript(script: string, extra: readonly string[] = []): string[] { + // pnpm + yarn accept the bare-script shortcut (`pnpm dev`), npm + // does not. Using `run