From e3a026802bc8037afe025ecd8f42ce54c811f26d Mon Sep 17 00:00:00 2001 From: olaservo Date: Wed, 12 Aug 2026 09:04:31 -0700 Subject: [PATCH] fix: resolve node bins via package.json instead of spawning npx .cmd shims so the verify/smoke scripts run on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows npx/npm are .cmd shims a shell-free execFileSync/spawnSync cannot start (ENOENT since the CVE-2024-27980 hardening), so verify:typecheck-coverage died at validate's second step — doubly silently, echoing "(no diagnostic captured)" per project and then reporting all 918 tracked files as getting no tsc pass — and verify:build-gate, the smoke test-server bootstraps, and pack:verify's npm pack failed the same way. Add scripts/lib/resolve-node-bin.mjs: resolves the JS entry behind a package's bin from /package.json (deep resolution is blocked by Vite 8's exports map) and spawns it via process.execPath — the same walk npx --no-install did, cross-platform and shell-free. Switch the six npx call sites to it; the npm pack call gets the existing shell-on-win32 idiom instead (npm has no in-tree package to resolve). An unresolvable tsc is now a hard "cannot measure" error with actionable stderr, pinned by a new main() regression test. Closes #1939 Co-Authored-By: Claude Fable 5 --- scripts/lib/resolve-node-bin.mjs | 41 +++++++ scripts/lib/resolve-node-bin.test.mjs | 68 ++++++++++++ scripts/pack-and-verify.mjs | 4 +- scripts/smoke-cli.mjs | 17 ++- scripts/smoke-tui.mjs | 17 ++- scripts/smoke-web-app.mjs | 17 ++- scripts/verify-build-gate.mjs | 34 ++++-- .../verify-typecheck-coverage.main.test.mjs | 105 ++++++++++++++++++ scripts/verify-typecheck-coverage.mjs | 33 +++++- 9 files changed, 309 insertions(+), 27 deletions(-) create mode 100644 scripts/lib/resolve-node-bin.mjs create mode 100644 scripts/lib/resolve-node-bin.test.mjs create mode 100644 scripts/verify-typecheck-coverage.main.test.mjs diff --git a/scripts/lib/resolve-node-bin.mjs b/scripts/lib/resolve-node-bin.mjs new file mode 100644 index 000000000..528611c80 --- /dev/null +++ b/scripts/lib/resolve-node-bin.mjs @@ -0,0 +1,41 @@ +// Shared resolver for spawning a package's CLI cross-platform (#1939). +// +// On Windows, `npx`/`npm` are `.cmd` shims, not executables — a shell-free +// `execFileSync`/`spawnSync` cannot start one (Node refuses `.cmd`/`.bat` +// spawns without `shell: true` since the CVE-2024-27980 hardening) and throws +// `ENOENT`. GitHub CI runs Linux, so the gate stayed green there while being +// unrunnable for any Windows contributor. Instead of shelling through `npx`, +// resolve the JS entry behind the package's bin and run it with +// `process.execPath`: cross-platform, shell-free (no quoting hazards), faster +// (no npx resolution), and pinned to the locally installed package exactly as +// `npx --no-install` was. + +import { readFileSync } from "node:fs"; +import { createRequire } from "node:module"; +import path from "node:path"; + +/** + * Absolute path of the JS entry behind a package's bin (e.g. typescript's + * `tsc`, vite's `vite`), resolved from `fromDir` up the node_modules tree — + * the same walk `npx --no-install` does, minus the `.cmd` shim a shell-free + * spawn can't start on Windows. Resolves `/package.json` and reads its + * `bin` field (what npx itself does) rather than resolving the bin path + * directly, because an `exports` map blocks deep resolution — Vite 8 doesn't + * export `./bin/vite.js`, so `require.resolve("vite/bin/vite.js")` throws + * `ERR_PACKAGE_PATH_NOT_EXPORTED` (`./package.json` is always exported). + * + * Throws if the package isn't installed from `fromDir` or declares no such + * bin — the caller decides whether that's a hard "cannot measure" error or a + * fallback. + */ +export function resolveNodeBin(pkg, binName, fromDir) { + const pkgPath = createRequire(path.join(fromDir, "package.json")).resolve( + `${pkg}/package.json`, + ); + const bin = JSON.parse(readFileSync(pkgPath, "utf8")).bin; + // A string-form `bin` names a single command (the package's own name). + const rel = typeof bin === "string" ? bin : bin?.[binName]; + if (typeof rel !== "string") + throw new Error(`${pkg} declares no "${binName}" bin in its package.json`); + return path.join(path.dirname(pkgPath), rel); +} diff --git a/scripts/lib/resolve-node-bin.test.mjs b/scripts/lib/resolve-node-bin.test.mjs new file mode 100644 index 000000000..cac917e2e --- /dev/null +++ b/scripts/lib/resolve-node-bin.test.mjs @@ -0,0 +1,68 @@ +// Tests for `resolve-node-bin.mjs` (#1939) — the shared resolver that replaces +// shelling through `npx`, which is a `.cmd` shim on Windows that a shell-free +// `execFileSync`/`spawnSync` cannot start (ENOENT). Resolution is exercised +// against the packages the callers actually spawn (typescript, vite, prettier), +// installed by the repo's own `npm install`, so the contract is pinned against +// the real `bin`/`exports` shapes rather than fixtures that can drift. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { existsSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { resolveNodeBin } from "./resolve-node-bin.mjs"; + +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "..", + "..", +); + +test("resolves typescript's tsc (object-form bin) to an existing JS entry", () => { + const entry = resolveNodeBin("typescript", "tsc", repoRoot); + assert.ok(path.isAbsolute(entry), entry); + assert.ok(existsSync(entry), `resolved entry does not exist: ${entry}`); + assert.match(entry.split(path.sep).join("/"), /\/typescript\/.*tsc/); +}); + +test("resolves from a client dir, walking node_modules up like `npx --no-install`", () => { + const entry = resolveNodeBin( + "typescript", + "tsc", + path.join(repoRoot, "clients", "cli"), + ); + assert.ok(existsSync(entry), `resolved entry does not exist: ${entry}`); +}); + +test("resolves vite's bin despite Vite 8's exports map (no deep bin export)", () => { + // `require.resolve("vite/bin/vite.js")` throws ERR_PACKAGE_PATH_NOT_EXPORTED + // under Vite 8 — the reason the helper goes through `/package.json`. + const entry = resolveNodeBin( + "vite", + "vite", + path.join(repoRoot, "clients", "web"), + ); + assert.ok(existsSync(entry), `resolved entry does not exist: ${entry}`); + assert.match(entry.split(path.sep).join("/"), /\/vite\/.*vite\.js$/); +}); + +test("resolves a string-form bin (prettier), ignoring the binName", () => { + const entry = resolveNodeBin("prettier", "prettier", repoRoot); + assert.ok(existsSync(entry), `resolved entry does not exist: ${entry}`); + assert.match(entry.split(path.sep).join("/"), /\/prettier\//); +}); + +test("throws when the package is not installed from fromDir", () => { + assert.throws( + () => resolveNodeBin("definitely-not-installed-anywhere", "x", repoRoot), + /definitely-not-installed-anywhere/, + ); +}); + +test("throws when the package declares no such bin", () => { + // typescript's bin map has `tsc`/`tsserver`, not `vite`. + assert.throws( + () => resolveNodeBin("typescript", "vite", repoRoot), + /typescript declares no "vite" bin/, + ); +}); diff --git a/scripts/pack-and-verify.mjs b/scripts/pack-and-verify.mjs index ff1dd310e..2cd4494d4 100644 --- a/scripts/pack-and-verify.mjs +++ b/scripts/pack-and-verify.mjs @@ -137,7 +137,9 @@ step("packing the publishable tarball (npm pack)..."); const pack = spawnSync( "npm", ["pack", "--json", "--ignore-scripts", "--pack-destination", tmpdir()], - { cwd: repoRoot, encoding: "utf8" }, + // npm is npm.cmd on Windows, which needs a shell to resolve (#1939) — the + // same idiom as runInherit/runBin below. + { cwd: repoRoot, encoding: "utf8", shell: process.platform === "win32" }, ); if (pack.status !== 0) { fail(`\`npm pack\` failed:\n${pack.stderr || pack.stdout}`); diff --git a/scripts/smoke-cli.mjs b/scripts/smoke-cli.mjs index ef93bdfdf..80660208b 100644 --- a/scripts/smoke-cli.mjs +++ b/scripts/smoke-cli.mjs @@ -44,6 +44,7 @@ import { mkdtempSync, rmSync, existsSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { pathToFileURL } from "node:url"; +import { resolveNodeBin } from "./lib/resolve-node-bin.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); const launcher = join(repoRoot, "clients", "launcher", "build", "index.js"); @@ -69,10 +70,18 @@ function fail(message) { function ensureTestServer() { if (existsSync(testServer) && existsSync(httpTestServerModule)) return; console.log("smoke:cli — building test-servers (missing build output)..."); - const r = spawnSync("npx", ["tsc", "-p", "test-servers", "--noCheck"], { - cwd: repoRoot, - stdio: "inherit", - }); + // The root-installed tsc, run via this Node — `npx` is a `.cmd` shim on + // Windows that a shell-free spawnSync can't start (ENOENT — #1939). + const r = spawnSync( + process.execPath, + [ + resolveNodeBin("typescript", "tsc", repoRoot), + "-p", + "test-servers", + "--noCheck", + ], + { cwd: repoRoot, stdio: "inherit" }, + ); if ( r.status !== 0 || !existsSync(testServer) || diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index d6244cd3e..bf2a8e2d0 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -24,6 +24,7 @@ import { mkdtempSync, existsSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { removeSafe } from "./lib/child-cleanup.mjs"; +import { resolveNodeBin } from "./lib/resolve-node-bin.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); const launcher = join(repoRoot, "clients", "launcher", "build", "index.js"); @@ -44,10 +45,18 @@ function fail(message) { function ensureTestServer() { if (existsSync(testServer)) return; console.log("smoke:tui — building test-servers (missing build output)..."); - const r = spawnSync("npx", ["tsc", "-p", "test-servers", "--noCheck"], { - cwd: repoRoot, - stdio: "inherit", - }); + // The root-installed tsc, run via this Node — `npx` is a `.cmd` shim on + // Windows that a shell-free spawnSync can't start (ENOENT — #1939). + const r = spawnSync( + process.execPath, + [ + resolveNodeBin("typescript", "tsc", repoRoot), + "-p", + "test-servers", + "--noCheck", + ], + { cwd: repoRoot, stdio: "inherit" }, + ); if (r.status !== 0 || !existsSync(testServer)) { fail( "could not build the stdio test server (test-servers/build/test-server-stdio.js). " + diff --git a/scripts/smoke-web-app.mjs b/scripts/smoke-web-app.mjs index c5f2f8a60..4dafa18f9 100644 --- a/scripts/smoke-web-app.mjs +++ b/scripts/smoke-web-app.mjs @@ -53,6 +53,7 @@ import { setTimeout as delay } from "node:timers/promises"; import { join, resolve } from "node:path"; import { startProdWebServer } from "./lib/prod-web-server.mjs"; import { stopChild } from "./lib/child-cleanup.mjs"; +import { resolveNodeBin } from "./lib/resolve-node-bin.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); const requireFromWeb = createRequire( @@ -140,10 +141,18 @@ function ensureTestServer() { console.log( "smoke:web:app — building test-servers (missing build output)...", ); - const r = spawnSync("npx", ["tsc", "-p", "test-servers", "--noCheck"], { - cwd: repoRoot, - stdio: "inherit", - }); + // The root-installed tsc, run via this Node — `npx` is a `.cmd` shim on + // Windows that a shell-free spawnSync can't start (ENOENT — #1939). + const r = spawnSync( + process.execPath, + [ + resolveNodeBin("typescript", "tsc", repoRoot), + "-p", + "test-servers", + "--noCheck", + ], + { cwd: repoRoot, stdio: "inherit" }, + ); if (r.status !== 0 || !existsSync(composableServer)) { throw new Error( "could not build the test servers (test-servers/build/server-composable.js). " + diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 007e556e8..350d62f98 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -32,6 +32,7 @@ import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { resolveNodeBin } from "./lib/resolve-node-bin.mjs"; const repoRoot = path.resolve( path.dirname(fileURLToPath(import.meta.url)), @@ -82,6 +83,20 @@ function escapeRegExp(literal) { return literal.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } +// The repo-pinned Vite's JS entry, resolved from clients/web — the old `npx +// --no-install` guarantee (never a registry fetch) — and spawned via this Node, +// since `npx` is a `.cmd` shim on Windows that a shell-free spawnSync can't +// start (ENOENT — #1939). Resolved up front, BEFORE the probe is injected, so a +// missing install fails actionably without ever touching src/main.tsx. +let viteEntry; +try { + viteEntry = resolveNodeBin("vite", "vite", webDir); +} catch (err) { + fail( + `cannot resolve \`vite\` from clients/web (${err.message}) — run \`npm install\` at the repo root first`, + ); +} + // Write the captured original to a backup and fail — the honest remedy when the // in-place restore can't be trusted, since it preserves any uncommitted edits // the developer had (unlike `git checkout --`). The backup goes in a fresh @@ -229,15 +244,14 @@ try { console.log( "verify:build-gate: running a real `vite build` with a node:fs probe (takes a minute)…", ); - // `--no-install` pins to the locally installed (repo-pinned) Vite: the whole - // point is proving the message-keyed gate fires against THIS Vite, so `npx` - // must never silently fetch a different version from the registry when - // clients/web/node_modules is missing/partial. A missing local bin then - // surfaces via the `result.error` check below. `timeout` bounds a hung build: - // spawnSync sets `result.error` (ETIMEDOUT) on timeout, so the same branch - // reports it — otherwise a hang would burn to the GitHub job's 360-min default - // with no output (this step captures rather than inherits stdio). - result = spawnSync("npx", ["--no-install", "vite", "build"], { + // `viteEntry` (resolved above) pins to the locally installed (repo-pinned) + // Vite: the whole point is proving the message-keyed gate fires against THIS + // Vite, so nothing may silently fetch a different version from the registry + // when clients/web/node_modules is missing/partial. `timeout` bounds a hung + // build: spawnSync sets `result.error` (ETIMEDOUT) on timeout, so that branch + // below reports it — otherwise a hang would burn to the GitHub job's 360-min + // default with no output (this step captures rather than inherits stdio). + result = spawnSync(process.execPath, [viteEntry, "build"], { cwd: webDir, encoding: "utf8", timeout: 10 * 60_000, @@ -266,7 +280,7 @@ if (afterRestore !== original) { ); } -// A spawn failure (e.g. `npx` missing) leaves `status` null with no output — +// A spawn failure (or the timeout above) leaves `status` null with no output — // surface it as itself rather than falling through to the "not via the gate" // diagnosis, which would send someone chasing a build regression that isn't real. if (result.error) { diff --git a/scripts/verify-typecheck-coverage.main.test.mjs b/scripts/verify-typecheck-coverage.main.test.mjs new file mode 100644 index 000000000..e7ac5ac89 --- /dev/null +++ b/scripts/verify-typecheck-coverage.main.test.mjs @@ -0,0 +1,105 @@ +// Regression test for #1939's "doubly silent" failure mode: when the tsc entry +// cannot be resolved (on Windows the old `execFileSync("npx", …)` threw ENOENT; +// today, a missing install), the guard must hard-fail with an actionable +// "cannot measure" error — NOT swallow it, echo "(no diagnostic captured)" per +// project, and then report every tracked source file in the repo as getting no +// tsc pass, which is what shipped before and sent Windows contributors chasing +// a 900-file coverage regression that wasn't real. +// +// The fixture is a throwaway repo with one enrolled client whose `typecheck` +// names a project, but with NO node_modules anywhere up the temp tree — so the +// guard reaches the tsc-entry resolution and it fails. Mirrors the +// `verify-format-coverage.main.test.mjs` / `verify-dep-lockstep.main.test.mjs` +// pattern. Run via `npm run test:scripts`. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { execFileSync, spawnSync } from "node:child_process"; +import { + cpSync, + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const scriptsDir = path.dirname(fileURLToPath(import.meta.url)); + +test("unresolvable tsc is a hard 'cannot measure' error, not an empty file set", () => { + // realpath'd because the script only executes when `import.meta.url` matches + // `process.argv[1]`, and macOS `tmpdir()` is a symlink — see the note in + // `verify-dep-lockstep.main.test.mjs`. + const dir = realpathSync(mkdtempSync(path.join(tmpdir(), "typecheck-cov-"))); + try { + // Root manifest: the full guard cycle is wired so phase 1 gets as far as + // measuring the client (an unwired client is `continue`d past, and the + // resolution would never be reached). + writeFileSync( + path.join(dir, "package.json"), + JSON.stringify({ + name: "fixture", + scripts: { + validate: + "npm run verify:format-coverage && npm run verify:typecheck-coverage && npm run verify:dep-lockstep && npm run test:scripts && npm --prefix clients/cli run validate", + "verify:format-coverage": "node scripts/verify-format-coverage.mjs", + "verify:typecheck-coverage": + "node scripts/verify-typecheck-coverage.mjs", + "verify:dep-lockstep": "node scripts/verify-dep-lockstep.mjs", + "test:scripts": 'node --test "scripts/**/*.test.mjs"', + }, + }), + ); + // One enrolled client with a `typecheck` reachable from `validate`, a + // project for it to name, and a tracked source file — so if the hard error + // ever regresses to the old behavior, the "get no `tsc` pass" report the + // second assertion forbids would actually have a file to list. + mkdirSync(path.join(dir, "clients", "cli", "src"), { recursive: true }); + writeFileSync( + path.join(dir, "clients", "cli", "package.json"), + JSON.stringify({ + name: "fixture-cli", + scripts: { + validate: "npm run typecheck", + typecheck: "tsc --noEmit -p tsconfig.json", + }, + }), + ); + writeFileSync( + path.join(dir, "clients", "cli", "tsconfig.json"), + JSON.stringify({ include: ["src"] }), + ); + writeFileSync( + path.join(dir, "clients", "cli", "src", "index.ts"), + "export const x = 1;\n", + ); + mkdirSync(path.join(dir, "scripts", "lib"), { recursive: true }); + for (const rel of [ + "verify-typecheck-coverage.mjs", + path.join("lib", "npm-scripts.mjs"), + path.join("lib", "resolve-node-bin.mjs"), + ]) + cpSync(path.join(scriptsDir, rel), path.join(dir, "scripts", rel)); + execFileSync("git", ["init", "-q"], { cwd: dir }); + execFileSync("git", ["add", "-A"], { cwd: dir }); + + const r = spawnSync( + process.execPath, + [path.join(dir, "scripts", "verify-typecheck-coverage.mjs")], + { cwd: dir, encoding: "utf8" }, + ); + const out = `${r.stdout}${r.stderr}`; + assert.equal(r.status, 1, out); + assert.match(out, /cannot resolve `typescript` from clients\/cli/, out); + assert.match(out, /npm install/, out); + // The pre-#1939 failure shape: per-project "(no diagnostic captured)" + // warnings followed by every tracked file reported uncovered. + assert.doesNotMatch(out, /get no `tsc` pass/, out); + assert.doesNotMatch(out, /no diagnostic captured/, out); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/scripts/verify-typecheck-coverage.mjs b/scripts/verify-typecheck-coverage.mjs index 20d6ab2cc..953123131 100644 --- a/scripts/verify-typecheck-coverage.mjs +++ b/scripts/verify-typecheck-coverage.mjs @@ -47,6 +47,7 @@ import { rootRunsClientValidate, tokenize, } from "./lib/npm-scripts.mjs"; +import { resolveNodeBin } from "./lib/resolve-node-bin.mjs"; const repoRoot = path.resolve( path.dirname(fileURLToPath(import.meta.url)), @@ -456,8 +457,8 @@ export function typecheckProjects(scripts) { function projectDisablesChecking(clientDir, project) { try { const out = execFileSync( - "npx", - ["--no-install", "tsc", "-p", project, "--showConfig"], + process.execPath, + [tscEntry(clientDir), "-p", project, "--showConfig"], { cwd: path.join(repoRoot, clientDir), encoding: "utf8" }, ); return JSON.parse(out)?.compilerOptions?.noCheck === true; @@ -473,6 +474,30 @@ function projectDisablesChecking(clientDir, project) { * in the set but are harmless — the set is only ever queried with client-relative * paths. Cached: `resolveLeafProjects` and `projectFiles` both list a project. */ +// The tsc JS entry each client's projects are measured with, resolved from the +// client dir up the node_modules tree exactly as `npx --no-install tsc` walked +// — but spawnable shell-free on Windows, where `npx` is a `.cmd` shim that +// `execFileSync` can't start (ENOENT — #1939). A resolution failure is a hard +// "cannot measure" error rather than an empty file set: the old ENOENT was +// doubly silent, echoing "(no diagnostic captured)" per project and then +// reporting every tracked file in the repo as uncovered. +const tscEntryCache = new Map(); +function tscEntry(clientDir) { + const cached = tscEntryCache.get(clientDir); + if (cached) return cached; + let entry; + try { + entry = resolveNodeBin("typescript", "tsc", path.join(repoRoot, clientDir)); + } catch (err) { + console.error( + `verify:typecheck-coverage — cannot resolve \`typescript\` from ${clientDir} (${err.message}): this guard cannot measure anything. Run \`npm install\` at the repo root first.`, + ); + process.exit(1); + } + tscEntryCache.set(clientDir, entry); + return entry; +} + const rawFilesCache = new Map(); function rawProjectFiles(clientDir, project) { const key = `${clientDir}|${project}`; @@ -482,8 +507,8 @@ function rawProjectFiles(clientDir, project) { let stdout; try { stdout = execFileSync( - "npx", - ["--no-install", "tsc", "-p", project, "--listFilesOnly"], + process.execPath, + [tscEntry(clientDir), "-p", project, "--listFilesOnly"], { cwd: absClient, encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }, ); } catch (err) {