diff --git a/.changeset/create-objectstack-honest-pm-probe.md b/.changeset/create-objectstack-honest-pm-probe.md new file mode 100644 index 0000000000..e244cd353f --- /dev/null +++ b/.changeset/create-objectstack-honest-pm-probe.md @@ -0,0 +1,35 @@ +--- +"create-objectstack": patch +--- + +fix(create-objectstack): stop reporting a failed pnpm probe as a deliberate npm choice (#11616) + +`detectPackageManager()` was `try { execSync('pnpm --version') } catch { return +'npm' }`, so every failure mode collapsed into one answer. `npm install` in the +scaffolder's output meant either *this machine has no pnpm* or *the probe +threw*, and nothing — no log line, no message — could tell the two apart. + +That second case is reachable on an ordinary developer machine, not just in +theory: `pnpm --version` resolves through Corepack and therefore depends on the +directory it runs in. Measured on one machine, one binary, two directories — +`10.31.0` inside a repo that pins `packageManager`, `10.33.0` outside it, where +Corepack has to resolve, and may have to fetch, a version nothing pinned. A +user who has pnpm installed but is on a slow or offline network was silently +told to run npm. + +The probe now reports why as well as what: + +- `probe: 'ok'` — pnpm answered, so pnpm is used (unchanged, silent). +- `probe: 'absent'` — no pnpm on PATH at all, so npm is a real choice + (unchanged, silent). +- `probe: 'failed'` — pnpm **is** on PATH and the probe still threw. npm is + used exactly as before, and the run now says so, naming the underlying + failure: `pnpm is installed but \`pnpm --version\` failed (); using + npm as a fallback.` + +**Which package manager a run uses is unchanged in all three cases** — it is +still pnpm if and only if the probe succeeded. The PATH lookup that separates +`absent` from `failed` runs only after the decision is already made and feeds +the message alone, so a miss there can change a warning's wording and never the +tool's behaviour. The only output that moves is one warning in a case that was +previously silent and wrong. diff --git a/packages/create-objectstack/src/detect-package-manager.test.ts b/packages/create-objectstack/src/detect-package-manager.test.ts new file mode 100644 index 0000000000..159e487bdb --- /dev/null +++ b/packages/create-objectstack/src/detect-package-manager.test.ts @@ -0,0 +1,187 @@ +// Copyright (c) 2026 ObjectStack contributors. Apache-2.0 license. +// +// Pins the package-manager probe's VERDICT — both what it decides and why. +// +// The card this file answers is a flake, but the flake was a symptom. The old +// detector was `try { execSync('pnpm --version') } catch { return 'npm' }`, so +// `npm` in a transcript meant either "this machine has no pnpm" or "the probe +// threw" and nothing could tell which. `pnpm --version` resolves through +// Corepack and therefore depends on the cwd it runs in, so the second case is +// reachable on any machine with a slow or offline network — which is how a +// merge-queue job on a diff that could not reach this package went red. +// +// Every test here is hermetic by construction: both ambient reads (the probe +// and the PATH lookup) are injected, so nothing in this file can be decided by +// the runner. That is deliberate and it is the point of the card — a pin that +// asks the environment a question it cannot pin the answer to is measuring the +// runner, not the code. + +import { describe, it, expect } from 'vitest'; +import path from 'node:path'; +import fs from 'node:fs'; +import os from 'node:os'; +import { + detectPackageManager, + probeFailureDetail, + resolveOnPath, +} from './detect-package-manager.js'; + +/** A probe that fails the way a real `execSync` failure does. */ +function throwingProbe(err: unknown): () => void { + return () => { + throw err; + }; +} + +/** The shape `execSync` throws: a status, and stderr only if it was piped. */ +function execError(over: Record = {}): Error { + return Object.assign(new Error('Command failed: pnpm --version'), { + status: 1, + signal: null, + stderr: Buffer.from(''), + ...over, + }); +} + +describe('detectPackageManager — the decision', () => { + it('probe succeeds -> pnpm', () => { + const out = detectPackageManager({ probe: () => {}, pnpmOnPath: () => true }); + expect(out).toEqual({ pm: 'pnpm', probe: 'ok' }); + }); + + it('probe throws, pnpm absent from PATH -> npm', () => { + const out = detectPackageManager({ + probe: throwingProbe(execError({ status: 127 })), + pnpmOnPath: () => false, + }); + expect(out).toEqual({ pm: 'npm', probe: 'absent' }); + }); + + it('probe throws, pnpm present on PATH -> npm', () => { + const out = detectPackageManager({ + probe: throwingProbe(execError({ stderr: Buffer.from('Error: getaddrinfo ENOTFOUND registry.npmjs.org\n') })), + pnpmOnPath: () => true, + }); + expect(out.pm).toBe('npm'); + }); + + // Clause-② guard for this change: the change was allowed to move what the + // tool REPORTS, never what it DOES. `pm` must still be a pure function of + // "did the probe succeed", exactly as the collapsed version was — the PATH + // lookup must not be able to move it. Green before and after the fix by + // design; a regression guard, not evidence the fix was needed. + it('regression guard: pm is pnpm if and only if the probe succeeded', () => { + for (const pnpmOnPath of [true, false]) { + expect(detectPackageManager({ probe: () => {}, pnpmOnPath: () => pnpmOnPath }).pm).toBe('pnpm'); + expect( + detectPackageManager({ probe: throwingProbe(execError()), pnpmOnPath: () => pnpmOnPath }).pm, + ).toBe('npm'); + } + }); +}); + +describe('detectPackageManager — the distinction that used to be collapsed', () => { + // THE collapse guard. Both of these answer `npm`; if a future edit folds the + // two failure modes back into one answer, these two objects become equal and + // this test goes red. Asserting only `pm` cannot catch that — that is the + // whole defect — so the assertion is on the reason. + it('"probe threw" and "chose npm" are different verdicts, not one', () => { + const absent = detectPackageManager({ + probe: throwingProbe(execError({ status: 127 })), + pnpmOnPath: () => false, + }); + const failed = detectPackageManager({ + probe: throwingProbe(execError({ stderr: Buffer.from('corepack: fetch failed\n') })), + pnpmOnPath: () => true, + }); + + expect(absent.pm).toBe(failed.pm); // same decision... + expect(absent.probe).not.toBe(failed.probe); // ...different reason + expect(absent.probe).toBe('absent'); + expect(failed.probe).toBe('failed'); + expect(absent).not.toEqual(failed); + }); + + it('only the "probe threw" verdict carries a detail to report', () => { + const failed = detectPackageManager({ + probe: throwingProbe(execError({ stderr: Buffer.from('corepack: fetch failed\n') })), + pnpmOnPath: () => true, + }); + expect(failed).toHaveProperty('detail', 'corepack: fetch failed'); + + const absent = detectPackageManager({ + probe: throwingProbe(execError({ status: 127 })), + pnpmOnPath: () => false, + }); + expect(absent).not.toHaveProperty('detail'); + + const ok = detectPackageManager({ probe: () => {}, pnpmOnPath: () => true }); + expect(ok).not.toHaveProperty('detail'); + }); + + it('the PATH lookup is not consulted when the probe succeeds', () => { + let consulted = false; + detectPackageManager({ + probe: () => {}, + pnpmOnPath: () => { + consulted = true; + return true; + }, + }); + expect(consulted).toBe(false); + }); +}); + +describe('probeFailureDetail — one bounded line, most specific evidence first', () => { + it('names the signal when the child was killed', () => { + expect(probeFailureDetail(execError({ signal: 'SIGTERM', stderr: Buffer.from('noise\n') }))) + .toBe('killed by SIGTERM'); + }); + + it("uses stderr's first non-empty line when there is one", () => { + expect(probeFailureDetail(execError({ stderr: Buffer.from('\n\n corepack: fetch failed \nmore\n') }))) + .toBe('corepack: fetch failed'); + }); + + it('falls back to a libuv code, then to the exit status', () => { + expect(probeFailureDetail(execError({ code: 'ENOENT', status: null }))).toBe('ENOENT'); + expect(probeFailureDetail(execError({ status: 127 }))).toBe('exited 127'); + }); + + it('never returns a multi-line or unbounded string — it lands in a console warning', () => { + const detail = probeFailureDetail(execError({ stderr: Buffer.from(`${'x'.repeat(5000)}\nsecond\n`) })); + expect(detail).not.toContain('\n'); + expect(detail.length).toBeLessThanOrEqual(200); + }); + + it('degrades to a fixed string rather than throwing on a non-Error', () => { + expect(probeFailureDetail(undefined)).toBe('unknown error'); + expect(probeFailureDetail(null)).toBe('unknown error'); + }); +}); + +describe('resolveOnPath — the PATH read, without spawning', () => { + it('finds an executable in an earlier PATH entry and returns its full path', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'create-objectstack-pathprobe-')); + try { + const bin = path.join(dir, 'pnpm'); + fs.writeFileSync(bin, '#!/bin/sh\nexit 0\n', { mode: 0o755 }); + expect(resolveOnPath('pnpm', { PATH: `${dir}${path.delimiter}/nonexistent` })).toBe(bin); + expect(resolveOnPath('pnpm', { PATH: '/nonexistent' })).toBeNull(); + expect(resolveOnPath('pnpm', { PATH: '' })).toBeNull(); + expect(resolveOnPath('pnpm', {})).toBeNull(); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does not mistake a directory of the same name for an executable', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'create-objectstack-pathprobe-')); + try { + fs.mkdirSync(path.join(dir, 'pnpm')); + expect(resolveOnPath('pnpm', { PATH: dir })).toBeNull(); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/create-objectstack/src/detect-package-manager.ts b/packages/create-objectstack/src/detect-package-manager.ts new file mode 100644 index 0000000000..b4b16f33db --- /dev/null +++ b/packages/create-objectstack/src/detect-package-manager.ts @@ -0,0 +1,140 @@ +// Copyright (c) 2026 ObjectStack contributors. Apache-2.0 license. +// +// The package-manager probe, lifted out of index.ts so it can be tested +// without importing that module (which calls `program.parse()` at import +// time) and without spawning anything — the same reason pkg-utils.ts, +// rewrite-identity.ts and created-summary.ts live outside index.ts. +// +// WHY THE OUTCOME IS A RECORD AND NOT A BARE STRING +// +// The probe used to be `try { execSync('pnpm --version') } catch { return +// 'npm' }`, which collapses every failure mode into one answer. `npm` in the +// output then meant *the probe threw*, not *the code chose npm*, and nothing +// downstream — no log line, no assertion message — could tell the two apart. +// +// That is not only a diagnostics problem. `pnpm --version` resolves through +// Corepack, so it depends on the cwd it runs in: measured on one machine, one +// binary, two directories, `pnpm --version` answered 10.31.0 inside this repo +// (the pinned `packageManager`) and 10.33.0 outside it, where Corepack has to +// resolve — and may have to FETCH — a version nothing pinned. A user who has +// pnpm installed, on a slow or offline network, was silently told to run npm. +// +// So the decision and the reason are now separate values. The DECISION is +// byte-for-byte the old one — `pnpm` if the probe succeeds, `npm` otherwise — +// because which package manager actually runs is not a thing this change is +// entitled to move. Only the REASON is new, and it distinguishes the two +// cases that were collapsed: +// +// probe: 'ok' the probe succeeded -> pnpm +// probe: 'absent' no pnpm on PATH at all -> npm, a real choice +// probe: 'failed' pnpm IS on PATH, probe threw -> npm, a FALLBACK +// +// Only 'failed' is new information, and only 'failed' prints anything extra: +// on every path that was already correct the output is unchanged. + +import fs from 'node:fs'; +import path from 'node:path'; +import { execSync } from 'node:child_process'; + +export type PackageManagerDetection = + | { pm: 'pnpm'; probe: 'ok' } + | { pm: 'npm'; probe: 'absent' } + | { pm: 'npm'; probe: 'failed'; detail: string }; + +/** The two ambient reads this module makes, injectable so tests can be hermetic. */ +export interface DetectDeps { + /** Runs the version probe. Returns normally on success, throws on any failure. */ + probe: () => void; + /** Whether a `pnpm` executable is resolvable on PATH at all. */ + pnpmOnPath: () => boolean; +} + +/** + * Resolve an executable on PATH without spawning anything. + * + * Deliberately NOT a second subprocess: this runs only after the probe has + * already failed, and a machine whose probe just failed is the last place to + * spend another spawn. It also cannot change which package manager is chosen + * — it feeds the reason field only — so a miss here degrades a warning's + * wording and never the tool's behaviour. + */ +export function resolveOnPath(cmd: string, env: NodeJS.ProcessEnv = process.env): string | null { + const raw = env.PATH ?? ''; + if (!raw) return null; + // PATHEXT is Windows' list of what counts as executable; pnpm ships there as + // `pnpm.cmd`, so a bare-name check would miss it. Elsewhere the name is the + // whole story. + const exts = process.platform === 'win32' + ? (env.PATHEXT ?? '.COM;.EXE;.BAT;.CMD').split(';').filter(Boolean) + : ['']; + for (const dir of raw.split(path.delimiter)) { + if (!dir) continue; + for (const ext of exts) { + const candidate = path.join(dir, cmd + ext); + try { + if (fs.statSync(candidate).isFile()) return candidate; + } catch { + // Unreadable or missing entry — just not a hit. + } + } + } + return null; +} + +/** + * A one-line, log-safe description of why the probe threw. + * + * Order matters: a killed child reports its signal and no useful stderr, and a + * child that never launched reports a libuv code and no status — so the most + * specific evidence available is taken first and everything is flattened to a + * single bounded line, because this ends up inside a console warning. + */ +export function probeFailureDetail(err: unknown): string { + const e = (err ?? {}) as { + signal?: string | null; + status?: number | null; + code?: string | number | null; + stderr?: Buffer | string | null; + message?: string; + }; + if (e.signal) return `killed by ${e.signal}`; + const stderr = e.stderr == null ? '' : String(e.stderr); + const firstLine = stderr.split('\n').map((l) => l.trim()).find((l) => l.length > 0); + if (firstLine) return firstLine.length > 200 ? `${firstLine.slice(0, 197)}...` : firstLine; + if (typeof e.code === 'string') return e.code; + if (typeof e.status === 'number') return `exited ${e.status}`; + const msg = (e.message ?? '').split('\n')[0]?.trim(); + return msg || 'unknown error'; +} + +/** The real probe: a read-only `pnpm --version`, silent on success and on failure. */ +function defaultProbe(): void { + // stdin/stdout ignored, stderr CAPTURED rather than ignored: execSync + // attaches it to the thrown error, which is the only way the warning can + // name what actually went wrong. An explicit triple keeps the child's + // stderr off this process's stderr, so a run that succeeds — or one that + // fails — still prints nothing except what this module chooses to print. + execSync('pnpm --version', { stdio: ['ignore', 'ignore', 'pipe'] }); +} + +/** + * Decide which package manager this run should name, and why. + * + * The `pm` field is exactly the old function's return value. `probe` is the + * new part, and is the only thing callers should branch on when deciding + * whether to explain themselves to the user. + */ +export function detectPackageManager(deps: Partial = {}): PackageManagerDetection { + const probe = deps.probe ?? defaultProbe; + const pnpmOnPath = deps.pnpmOnPath ?? (() => resolveOnPath('pnpm') !== null); + try { + probe(); + return { pm: 'pnpm', probe: 'ok' }; + } catch (err) { + // The decision is already made at this point and does not depend on + // anything below: the probe threw, so it is npm either way. What follows + // only chooses which of the two npm cases to report. + if (!pnpmOnPath()) return { pm: 'npm', probe: 'absent' }; + return { pm: 'npm', probe: 'failed', detail: probeFailureDetail(err) }; + } +} diff --git a/packages/create-objectstack/src/index.ts b/packages/create-objectstack/src/index.ts index 5611c26894..b7eec54823 100644 --- a/packages/create-objectstack/src/index.ts +++ b/packages/create-objectstack/src/index.ts @@ -59,6 +59,7 @@ import chalk from 'chalk'; import fs from 'node:fs'; import path from 'node:path'; import { execSync } from 'node:child_process'; +import { detectPackageManager } from './detect-package-manager.js'; import { fileURLToPath } from 'node:url'; import { syncObjectStackDeps } from './pkg-utils.js'; @@ -122,15 +123,6 @@ function printError(msg: string) { console.log(chalk.red(` ✗ ${msg}`)); } function printStep(msg: string) { console.log(chalk.yellow(` → ${msg}`)); } function printWarning(msg: string) { console.log(chalk.yellow(` ⚠ ${msg}`)); } -function detectPackageManager(): string { - try { - execSync('pnpm --version', { stdio: 'ignore' }); - return 'pnpm'; - } catch { - return 'npm'; - } -} - // ─── Loading: bundled (fs copy) ───────────────────────────────────── function loadBundled(templateDir: string, targetDir: string): string[] { @@ -437,7 +429,24 @@ const program = new Command() // no install to drive. Previously "Next steps" hardcoded `npm` regardless // of which package manager actually ran (#10322) — a newcomer who just // watched `pnpm install` run was then told `npm run dev`. - const pm = detectPackageManager(); + // + // The probe reports WHY as well as WHAT. `npm` used to mean two different + // things that nothing downstream could tell apart: "this machine has no + // pnpm" and "the pnpm probe threw". The second is not a choice, it is a + // fallback under uncertainty — `pnpm --version` resolves through Corepack + // and so can fail on a slow or offline network even though pnpm is + // installed — and a run that stays silent about it tells the reader to + // type npm without ever admitting it never found out. + const detected = detectPackageManager(); + const pm = detected.pm; + if (detected.probe === 'failed') { + printWarning( + `pnpm is installed but \`pnpm --version\` failed (${detected.detail}); ` + + 'using npm as a fallback. The commands below name npm because the ' + + 'probe did not answer, not because this project prefers it.', + ); + console.log(''); + } printKV('Environment', projectName); printKV('Namespace', namespace); diff --git a/packages/create-objectstack/src/scaffold-next-steps-pm.test.ts b/packages/create-objectstack/src/scaffold-next-steps-pm.test.ts index e72eb31880..d3ddd10ff6 100644 --- a/packages/create-objectstack/src/scaffold-next-steps-pm.test.ts +++ b/packages/create-objectstack/src/scaffold-next-steps-pm.test.ts @@ -3,28 +3,45 @@ // Pins #10322: the printed "Next steps" (and the install-failure remedy) must // name the SAME package manager the run actually detected — never a // hardcoded `npm` regardless of what ran. Before this fix, a newcomer whose -// install ran with `pnpm` (confirmed empirically: this scaffolder prefers -// pnpm and only falls back to npm when pnpm is unreachable — see -// `detectPackageManager()`) was told to run `npm run dev` / `npm run -// validate` afterwards — the third of the "three different answers" #10322 -// measured. `packages/cli/src/commands/init.ts`'s own "Next steps" already -// threads its detected `chosenPm` through; this file is the same contract for +// install ran with `pnpm` was told to run `npm run dev` / `npm run validate` +// afterwards — the third of the "three different answers" #10322 measured. +// `packages/cli/src/commands/init.ts`'s own "Next steps" already threads its +// detected `chosenPm` through; this file is the same contract for // `create-objectstack`. // // `index.ts` calls `program.parse()` at import time, so it cannot be // unit-tested directly — this exercises the real CLI end to end via `tsx`, // the same no-build subprocess pattern `scaffold-description.test.ts` uses. // `--skip-install` keeps every run here fast and offline: `detectPackageManager()` -// is a read-only ` --version` probe (see index.ts), so its result — and -// therefore what "Next steps" prints — does not depend on an install actually -// following it. The *real* install path (both the pnpm and npm-fallback -// cases) was additionally verified by hand against the built CLI; see this -// issue's PR body for the transcripts. +// is a read-only ` --version` probe, so its result — and therefore what +// "Next steps" prints — does not depend on an install actually following it. // -// Both branches of the detector are exercised by controlling PATH: -// - pnpm reachable -> "pnpm run dev" / "pnpm run validate" -// - pnpm unreachable -> "npm run dev" / "npm run validate" (the fallback -// this scaffolder has always had for machines without pnpm) +// ─── WHY EVERY LEG STUBS pnpm, INCLUDING THE ONE WHERE pnpm WORKS ─────────── +// +// This file used to run its pnpm leg against the ambient environment, and that +// made a merge-queue job go red on a diff that could not reach this package. +// The reason is worth keeping written down, because "just re-run it" would +// have buried it: +// +// - the scaffold child runs in an `mkdtemp` under `os.tmpdir()`, OUTSIDE the +// repo, so its `pnpm --version` does not see the repo's pinned +// `packageManager` and Corepack has to resolve — and can have to FETCH — +// its own default instead. Measured, one machine, one binary, two cwds: +// 10.31.0 inside this repo, 10.33.0 outside it. +// - the test's own sanity check ran in the TEST process, whose cwd is inside +// the repo. It therefore could not cover the child at all: it passed while +// the thing it was guarding failed. +// +// So a network hiccup on a runner decided this file's verdict. The fix is not +// a retry, a longer timeout or a skip — each of those keeps the test measuring +// the runner and just makes it complain less. Instead the probe's OUTCOME is +// the fixture: every leg runs under a PATH holding a stub `pnpm` whose exit +// status this file chooses. Nothing here can be decided by the network. +// +// The sanity guard is not relaxed by that — it is re-aimed and made stricter. +// It now runs under the PATH the CHILD is given (the cwd/PATH mismatch above +// was the bug) and pins resolution to the exact stub, so a leg whose fixture +// silently failed to take effect fails loudly instead of going vacuous. // // The "no bare npm when pnpm ran" assertion is deliberately a WORD-BOUNDARY // match, not a substring one: the literal text "pnpm run" itself contains the @@ -45,18 +62,77 @@ const REPO_ROOT = path.resolve(PKG_ROOT, '..', '..'); const TSX = path.join(REPO_ROOT, 'node_modules', '.bin', 'tsx'); const INDEX_TS = path.join(PKG_ROOT, 'src', 'index.ts'); +/** The stderr a stubbed failing probe emits — echoed back in the warning. */ +const PROBE_FAILURE_STDERR = 'corepack: Network request to https://registry.npmjs.org/pnpm failed'; + function which(cmd: string): string { return execFileSync('sh', ['-c', `command -v ${cmd}`], { encoding: 'utf8' }).trim(); } -/** A PATH entry with `node` + `npm` reachable and `pnpm` deliberately absent. */ -function makePnpmlessBin(): string { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'create-objectstack-nopnpm-bin-')); +/** A PATH entry with `node` + `npm` reachable — the floor every leg needs, since + * `tsx` itself could not launch without them. */ +function makeBin(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'create-objectstack-pm-bin-')); fs.symlinkSync(which('node'), path.join(dir, 'node')); fs.symlinkSync(which('npm'), path.join(dir, 'npm')); return dir; } +/** `node` + `npm` reachable and `pnpm` deliberately absent. */ +function makePnpmlessBin(): string { + return makeBin(); +} + +/** + * `node` + `npm` reachable and a stub `pnpm` whose `--version` exits with + * `exitCode`. This is what makes the verdict hermetic: the real Corepack + * resolution — the part that needs a network and a cwd inside the repo — never + * runs, so the branch under test is chosen by this file and not by the runner. + */ +function makeStubPnpmBin(exitCode: number): string { + const dir = makeBin(); + const script = + exitCode === 0 + ? '#!/bin/sh\necho "10.31.0"\nexit 0\n' + : `#!/bin/sh\necho "${PROBE_FAILURE_STDERR}" >&2\nexit ${exitCode}\n`; + fs.writeFileSync(path.join(dir, 'pnpm'), script, { mode: 0o755 }); + return dir; +} + +/** The PATH a scaffold child is given for a leg. */ +function childPath(bin: string): string { + return `${bin}:/usr/bin:/bin`; +} + +/** + * The vacuity guard, re-aimed at the PATH the CHILD receives rather than the + * test process's own. Asserting mere reachability is what let the old guard + * pass over a child that resolved something else entirely, so this pins the + * resolution to the exact stub on disk. + */ +function expectPnpmResolvesToStub(bin: string): void { + const resolved = execFileSync('sh', ['-c', 'command -v pnpm'], { + env: { ...process.env, PATH: childPath(bin) }, + encoding: 'utf8', + }).trim(); + expect(resolved).toBe(path.join(bin, 'pnpm')); +} + +/** The mirror guard: the fake PATH really does hide pnpm, and really does still + * expose node/npm — otherwise `tsx` itself could not launch. */ +function expectPnpmUnreachable(bin: string): void { + expect(() => + execFileSync('sh', ['-c', 'command -v pnpm'], { + env: { ...process.env, PATH: childPath(bin) }, + }), + ).toThrow(); + expect(() => + execFileSync('sh', ['-c', 'command -v node && command -v npm'], { + env: { ...process.env, PATH: childPath(bin) }, + }), + ).not.toThrow(); +} + /** * The "Next steps:" block of a run's stdout — deliberately narrower than the * whole transcript. The "Created files" listing above it names @@ -70,58 +146,64 @@ function nextStepsSection(stdout: string): string { return stdout.split('Next steps:')[1] ?? ''; } -/** Run the real CLI with --skip-install --skip-skills and return its stdout. */ -function runScaffold(env: NodeJS.ProcessEnv): string { +/** Run the real CLI with --skip-install --skip-skills under `bin`'s PATH. */ +function runScaffold(bin: string): string { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'create-objectstack-nextsteps-')); try { return execFileSync( TSX, [INDEX_TS, 'my-app', '--template', 'blank', '--skip-install', '--skip-skills'], - { cwd: tmp, env, encoding: 'utf8' }, + { cwd: tmp, env: { ...process.env, PATH: childPath(bin) }, encoding: 'utf8' }, ); } finally { fs.rmSync(tmp, { recursive: true, force: true }); } } +/** Run `body` against a freshly built bin dir, cleaning it up afterwards. */ +function withBin(make: () => string, body: (bin: string) => void): void { + const bin = make(); + try { + body(bin); + } finally { + fs.rmSync(bin, { recursive: true, force: true }); + } +} + describe('scaffolder "Next steps" names the package manager it actually detected (#10322)', () => { it('with pnpm on PATH: prints pnpm consistently, never bare npm', () => { - // Sanity: this container really does have pnpm reachable, or the - // "consistently pnpm" assertion below would be vacuous. - expect(() => which('pnpm')).not.toThrow(); - - const nextSteps = nextStepsSection(runScaffold(process.env)); - expect(nextSteps).toMatch(/\bpnpm run dev\b/); - expect(nextSteps).toMatch(/\bpnpm run validate\b/); - expect(nextSteps).not.toMatch(/\bnpm run\b/); - expect(nextSteps).not.toMatch(/\bnpm install\b/); + withBin(() => makeStubPnpmBin(0), (bin) => { + expectPnpmResolvesToStub(bin); + + const nextSteps = nextStepsSection(runScaffold(bin)); + expect(nextSteps).toMatch(/\bpnpm run dev\b/); + expect(nextSteps).toMatch(/\bpnpm run validate\b/); + expect(nextSteps).not.toMatch(/\bnpm run\b/); + expect(nextSteps).not.toMatch(/\bnpm install\b/); + }); }, 20_000); it('with pnpm unreachable: falls back to npm — consistently, not a stale pnpm mention', () => { - const bin = makePnpmlessBin(); - try { - // Sanity: the fake PATH really does hide pnpm (and really does still - // expose node/npm — otherwise tsx itself could not launch). - expect(() => - execFileSync('sh', ['-c', 'command -v pnpm'], { - env: { ...process.env, PATH: bin }, - }), - ).toThrow(); - - const nextSteps = nextStepsSection( - runScaffold({ ...process.env, PATH: `${bin}:/usr/bin:/bin` }), - ); + withBin(makePnpmlessBin, (bin) => { + expectPnpmUnreachable(bin); + + const nextSteps = nextStepsSection(runScaffold(bin)); expect(nextSteps).toMatch(/\bnpm run dev\b/); expect(nextSteps).toMatch(/\bnpm run validate\b/); expect(nextSteps).not.toMatch(/pnpm/); - } finally { - fs.rmSync(bin, { recursive: true, force: true }); - } + }); }, 20_000); it('both branches still name the unskippable validate step (#10322 pt. 3)', () => { - expect(runScaffold(process.env)).toMatch(/run validate/); - }, 20_000); + // Both, actually both — this used to run one ambient leg twice under a + // name that claimed two. + withBin(() => makeStubPnpmBin(0), (bin) => { + expect(runScaffold(bin)).toMatch(/\bpnpm run validate\b/); + }); + withBin(makePnpmlessBin, (bin) => { + expect(runScaffold(bin)).toMatch(/\bnpm run validate\b/); + }); + }, 40_000); it('the install-failure remedy also names the detected package manager, not a hardcoded npm', () => { const source = fs.readFileSync(INDEX_TS, 'utf8'); @@ -129,3 +211,51 @@ describe('scaffolder "Next steps" names the package manager it actually detected expect(source).not.toMatch(/Run `npm install` manually/); }); }); + +// ─── The verdict has to be honest, not just deterministic ─────────────────── +// +// A hermetic test over a detector that still collapses "I failed" into "npm" +// would pass every time while asserting something false. These are the pins +// that make the transcript distinguish the two, end to end through the real +// CLI — the unit-level pins live in detect-package-manager.test.ts. +describe('a failed pnpm probe is reported as a fallback, not as a choice', () => { + it('pnpm present but its probe fails: still npm, and the run SAYS the probe failed', () => { + withBin(() => makeStubPnpmBin(1), (bin) => { + // Same guard as the succeeding leg: pnpm really is reachable here. That + // is what separates this case from the one below. + expectPnpmResolvesToStub(bin); + + const stdout = runScaffold(bin); + + // The decision is unchanged — this change was never entitled to move + // which package manager a user is told to run. + const nextSteps = nextStepsSection(stdout); + expect(nextSteps).toMatch(/\bnpm run dev\b/); + expect(nextSteps).toMatch(/\bnpm run validate\b/); + + // ...but the transcript now admits why, and names the actual failure + // instead of swallowing it. + expect(stdout).toContain('using npm as a fallback'); + expect(stdout).toContain(PROBE_FAILURE_STDERR); + }); + }, 20_000); + + it('pnpm simply absent: npm with NO fallback warning — the two cases stay distinguishable', () => { + withBin(makePnpmlessBin, (bin) => { + expectPnpmUnreachable(bin); + + const stdout = runScaffold(bin); + expect(nextStepsSection(stdout)).toMatch(/\bnpm run dev\b/); + + // The collapse guard, end to end: if a future edit reports both npm + // cases the same way, one of these two tests goes red. + expect(stdout).not.toContain('using npm as a fallback'); + }); + }, 20_000); + + it('a succeeding probe never claims a fallback', () => { + withBin(() => makeStubPnpmBin(0), (bin) => { + expect(runScaffold(bin)).not.toContain('using npm as a fallback'); + }); + }, 20_000); +});