From e3a026802bc8037afe025ecd8f42ce54c811f26d Mon Sep 17 00:00:00 2001 From: olaservo Date: Wed, 12 Aug 2026 09:04:31 -0700 Subject: [PATCH 1/4] 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) { From 4b143ce043bf73ba6c1d62abe1201008be9d0d06 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 23:43:24 -0400 Subject: [PATCH 2/4] fix(scripts): validate the bin name and that its file exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review on #1997: - A string-form `bin` is npm's shorthand for ONE command, named after the package, so ignoring `binName` let a typo silently resolve the package's only executable while bypassing the documented "declares no such bin" failure. Match it against the manifest's unscoped name instead. - A declared bin whose file is absent (partial install) was returned unchecked. `process.execPath ` still spawns and exits 1 with empty stdout, which `rawProjectListing` records as "no diagnostic captured" — reproducing the bogus every-file-uncovered report this helper exists to eliminate. Throw here, where the remedy is actionable. Also restores `rawProjectListing`'s docblock, which the new `tscEntry` block had orphaned above itself. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN Signed-off-by: cliffhall --- scripts/lib/resolve-node-bin.mjs | 40 ++++++++++--- scripts/lib/resolve-node-bin.test.mjs | 81 ++++++++++++++++++++++++++- scripts/lib/tsc-program.mjs | 20 +++---- 3 files changed, 121 insertions(+), 20 deletions(-) diff --git a/scripts/lib/resolve-node-bin.mjs b/scripts/lib/resolve-node-bin.mjs index 528611c80..e631d3f25 100644 --- a/scripts/lib/resolve-node-bin.mjs +++ b/scripts/lib/resolve-node-bin.mjs @@ -10,7 +10,7 @@ // (no npx resolution), and pinned to the locally installed package exactly as // `npx --no-install` was. -import { readFileSync } from "node:fs"; +import { existsSync, readFileSync } from "node:fs"; import { createRequire } from "node:module"; import path from "node:path"; @@ -24,18 +24,42 @@ import path from "node:path"; * 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. + * Throws if the package isn't installed from `fromDir`, declares no such bin, + * or declares one whose file is missing — 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]; + const manifest = JSON.parse(readFileSync(pkgPath, "utf8")); + const { bin } = manifest; + // A string-form `bin` is npm's shorthand for ONE command, named after the + // package (unscoped). It does not make every requested `binName` valid, so + // match it rather than accepting whatever was asked for — otherwise a typo + // silently resolves the package's only executable instead of failing. + const rel = + typeof bin === "string" + ? unscopedName(manifest.name ?? pkg) === binName + ? bin + : undefined + : 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); + const entry = path.join(path.dirname(pkgPath), rel); + // A declared bin whose file is absent is a partial install, and it fails + // *silently* downstream: `process.execPath ` still spawns fine and + // exits 1 with nothing on stdout, which `rawProjectListing` records as "no + // diagnostic captured" and turns into the bogus every-file-uncovered report + // this helper exists to eliminate. Fail here, where the remedy is actionable. + if (!existsSync(entry)) + throw new Error( + `${pkg}'s "${binName}" bin points at a missing file: ${entry}`, + ); + return entry; +} + +/** `@scope/name` → `name`; an unscoped name is returned unchanged. */ +function unscopedName(name) { + return name.startsWith("@") ? name.slice(name.indexOf("/") + 1) : name; } diff --git a/scripts/lib/resolve-node-bin.test.mjs b/scripts/lib/resolve-node-bin.test.mjs index cac917e2e..6ec1e605f 100644 --- a/scripts/lib/resolve-node-bin.test.mjs +++ b/scripts/lib/resolve-node-bin.test.mjs @@ -7,7 +7,14 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { existsSync } from "node:fs"; +import { + existsSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { resolveNodeBin } from "./resolve-node-bin.mjs"; @@ -46,12 +53,21 @@ test("resolves vite's bin despite Vite 8's exports map (no deep bin export)", () assert.match(entry.split(path.sep).join("/"), /\/vite\/.*vite\.js$/); }); -test("resolves a string-form bin (prettier), ignoring the binName", () => { +test("resolves a string-form bin (prettier) under the package's own name", () => { 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("rejects a mismatched binName against a string-form bin", () => { + // npm's string shorthand declares ONE command, named after the package — so + // a typo must fail rather than silently resolving prettier's executable. + assert.throws( + () => resolveNodeBin("prettier", "prettierd", repoRoot), + /prettier declares no "prettierd" bin/, + ); +}); + test("throws when the package is not installed from fromDir", () => { assert.throws( () => resolveNodeBin("definitely-not-installed-anywhere", "x", repoRoot), @@ -66,3 +82,64 @@ test("throws when the package declares no such bin", () => { /typescript declares no "vite" bin/, ); }); + +// The remaining cases need a manifest shape the real installs don't have, so +// they run against a throwaway node_modules tree rather than a real package. +function fixtureDir(manifest, { createBinFile = false } = {}) { + const dir = mkdtempSync(path.join(tmpdir(), "resolve-node-bin-")); + const pkgDir = path.join(dir, "node_modules", manifest.name); + mkdirSync(pkgDir, { recursive: true }); + writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify(manifest), + "utf8", + ); + if (createBinFile) { + const rel = + typeof manifest.bin === "string" + ? manifest.bin + : Object.values(manifest.bin)[0]; + const target = path.join(pkgDir, rel); + mkdirSync(path.dirname(target), { recursive: true }); + writeFileSync(target, "", "utf8"); + } + // The consumer `package.json` `createRequire` is based at. + writeFileSync( + path.join(dir, "package.json"), + JSON.stringify({ name: "consumer" }), + "utf8", + ); + return dir; +} + +test("throws when a declared bin's file is missing (partial install)", () => { + // The silent case this helper exists to kill: `process.execPath ` + // spawns fine and exits 1 with empty stdout, which downstream reads as "no + // diagnostic captured" and reports every tracked file as uncovered. + const dir = fixtureDir({ name: "ghostpkg", bin: { ghost: "bin/ghost.js" } }); + try { + assert.throws( + () => resolveNodeBin("ghostpkg", "ghost", dir), + /ghostpkg's "ghost" bin points at a missing file/, + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("accepts a scoped package's string-form bin under its unscoped name", () => { + const dir = fixtureDir( + { name: "@scope/tool", bin: "bin/tool.js" }, + { createBinFile: true }, + ); + try { + const entry = resolveNodeBin("@scope/tool", "tool", dir); + assert.ok(existsSync(entry), `resolved entry does not exist: ${entry}`); + assert.throws( + () => resolveNodeBin("@scope/tool", "scope-tool", dir), + /declares no "scope-tool" bin/, + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/scripts/lib/tsc-program.mjs b/scripts/lib/tsc-program.mjs index 25ee7616b..7f3fc79ee 100644 --- a/scripts/lib/tsc-program.mjs +++ b/scripts/lib/tsc-program.mjs @@ -191,16 +191,6 @@ export function resolveLeafProjects(clientDir, project, seen = new Set()) { ); } -/** - * Every file ONE project's program resolves, as absolute POSIX paths and with no - * filtering at all — first-party sources, `node_modules` declarations, and the - * out-of-repo `lib.*.d.ts` — plus the config diagnostic if `tsc` exited - * non-zero. The two consumers want different slices of the files, so the slicing - * happens in {@link projectSourceFiles} / {@link projectPackageFiles} rather than - * here; they differ on the `error` too, which is why it is reported rather than - * only warned about. Memoized per (client, project): the same project is listed - * by `resolveLeafProjects` and again by whichever slice the caller asks for. - */ /** * The tsc JS entry a client's programs are measured with, resolved from the * client dir up the node_modules tree exactly as `npx --no-install tsc` walked @@ -229,6 +219,16 @@ export function tscEntry(clientDir) { return entry; } +/** + * Every file ONE project's program resolves, as absolute POSIX paths and with no + * filtering at all — first-party sources, `node_modules` declarations, and the + * out-of-repo `lib.*.d.ts` — plus the config diagnostic if `tsc` exited + * non-zero. The two consumers want different slices of the files, so the slicing + * happens in {@link projectSourceFiles} / {@link projectPackageFiles} rather than + * here; they differ on the `error` too, which is why it is reported rather than + * only warned about. Memoized per (client, project): the same project is listed + * by `resolveLeafProjects` and again by whichever slice the caller asks for. + */ const listingCache = new Map(); export function rawProjectListing(clientDir, project) { const key = `${clientDir}|${project}`; From b399bc64e14487a2e4bb9bbef4bc01f70062550f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 17 Aug 2026 00:01:51 -0400 Subject: [PATCH 3/4] fix(scripts): find the manifest without exports, quote shell args on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 2 (suppressed comments), all three real: - `require.resolve("/package.json")` is governed by the package's `exports`, and Node keeps no special case for `./package.json` — so a package declaring an `exports` map without it threw ERR_PACKAGE_PATH_NOT_EXPORTED even though its bin was installed and spawnable, a false "not installed" pointing at `npm install` for a package already on disk. Walk `resolve.paths()` instead, which never consults `exports`. The docblock claiming the opposite is corrected. - `shell: true` (needed at all on Windows to start the `.cmd` shims) makes Node hand `cmd.exe` one space-joined string, so an argument holding a path with a space splits in two. Every generated path in pack-and-verify lives under `tmpdir()`, which sits beneath the user profile. Quote them via `shellArgs()` at all four spawn sites, the command path included — the hazard predates this PR, so fix the class. - Document the new module: `scripts/lib/resolve-node-bin.mjs` in AGENTS.md's scripts/ tree, its tests in the README's `test:scripts` row. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN Signed-off-by: cliffhall --- AGENTS.md | 7 ++++- README.md | 2 +- scripts/lib/resolve-node-bin.mjs | 37 +++++++++++++++++----- scripts/lib/resolve-node-bin.test.mjs | 21 +++++++++++++ scripts/pack-and-verify.mjs | 44 +++++++++++++++++++++------ 5 files changed, 92 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4ba943ecb..4d901292a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -128,7 +128,12 @@ v2/main/ │ # pack-and-verify.mjs, and lib/ shared helpers │ # (tsc-program.mjs is the `tsc --listFilesOnly` │ # measurement both coverage guards read a program -│ # through — #1965). Prettier-gated via +│ # through — #1965; resolve-node-bin.mjs resolves a +│ # package's bin through its own package.json so the +│ # verify/smoke/pack scripts spawn it with +│ # `process.execPath` instead of the `npx`/`npm` +│ # `.cmd` shims Node refuses to spawn shell-free on +│ # Windows — #1939). Prettier-gated via │ # `format:check:scripts`; its own pure parsers are │ # unit-tested by `npm run test:scripts` (node --test). ├── specification/ # Build specification diff --git a/README.md b/README.md index e3330d41d..502fe0765 100644 --- a/README.md +++ b/README.md @@ -312,7 +312,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus two headless-Chromium smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | +| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus `scripts/lib/resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | diff --git a/scripts/lib/resolve-node-bin.mjs b/scripts/lib/resolve-node-bin.mjs index e631d3f25..33d223aed 100644 --- a/scripts/lib/resolve-node-bin.mjs +++ b/scripts/lib/resolve-node-bin.mjs @@ -18,20 +18,18 @@ 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). + * spawn can't start on Windows. Reads the package's manifest and takes the + * path out of 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`. * * Throws if the package isn't installed from `fromDir`, declares no such bin, * or declares one whose file is missing — 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 pkgPath = resolveManifest(pkg, fromDir); const manifest = JSON.parse(readFileSync(pkgPath, "utf8")); const { bin } = manifest; // A string-form `bin` is npm's shorthand for ONE command, named after the @@ -59,6 +57,29 @@ export function resolveNodeBin(pkg, binName, fromDir) { return entry; } +/** + * The package's own `package.json`, found by walking the same `node_modules` + * chain Node's resolver would from `fromDir` — deliberately NOT via + * `require.resolve("/package.json")`. + * + * That shortcut reads as safe and isn't: subpath resolution is governed by the + * package's `exports`, and a package may declare one that omits `./package.json` + * (Node dropped its special case for it, so there is no guaranteed export). Such + * a package throws `ERR_PACKAGE_PATH_NOT_EXPORTED` here even though its bin is + * installed and perfectly spawnable — a false "not installed" that would send + * someone to `npm install` for a package already on disk. `resolve.paths()` + * gives the search dirs without consulting `exports` at all. + */ +function resolveManifest(pkg, fromDir) { + const require = createRequire(path.join(fromDir, "package.json")); + // `resolve.paths` returns null only for builtins, which have no bin to find. + for (const dir of require.resolve.paths(pkg) ?? []) { + const candidate = path.join(dir, ...pkg.split("/"), "package.json"); + if (existsSync(candidate)) return candidate; + } + throw new Error(`cannot find ${pkg} from ${fromDir}`); +} + /** `@scope/name` → `name`; an unscoped name is returned unchanged. */ function unscopedName(name) { return name.startsWith("@") ? name.slice(name.indexOf("/") + 1) : name; diff --git a/scripts/lib/resolve-node-bin.test.mjs b/scripts/lib/resolve-node-bin.test.mjs index 6ec1e605f..e62a72ed1 100644 --- a/scripts/lib/resolve-node-bin.test.mjs +++ b/scripts/lib/resolve-node-bin.test.mjs @@ -127,6 +127,27 @@ test("throws when a declared bin's file is missing (partial install)", () => { } }); +test("resolves a package whose `exports` hides ./package.json", () => { + // `require.resolve("/package.json")` throws ERR_PACKAGE_PATH_NOT_EXPORTED + // for this shape — Node has no special case keeping `./package.json` + // exported — which would be a false "not installed" for a package whose bin + // is right there on disk. The manifest lookup walks node_modules instead. + const dir = fixtureDir( + { + name: "walledpkg", + exports: { ".": "./index.js" }, + bin: { walled: "bin/walled.js" }, + }, + { createBinFile: true }, + ); + try { + const entry = resolveNodeBin("walledpkg", "walled", dir); + assert.ok(existsSync(entry), `resolved entry does not exist: ${entry}`); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + test("accepts a scoped package's string-form bin under its unscoped name", () => { const dir = fixtureDir( { name: "@scope/tool", bin: "bin/tool.js" }, diff --git a/scripts/pack-and-verify.mjs b/scripts/pack-and-verify.mjs index 2cd4494d4..7a0deedfe 100644 --- a/scripts/pack-and-verify.mjs +++ b/scripts/pack-and-verify.mjs @@ -98,12 +98,28 @@ function step(message) { console.log(`\npack:verify — ${message}`); } +// Windows needs `shell: true` to start the `npm`/`npx` `.cmd` shims at all +// (#1939), and with a shell Node hands the argv to `cmd.exe` as ONE +// space-joined string — so an argument holding a path with a space is split +// into two. That is not hypothetical here: every generated path in this file +// lives under `tmpdir()`, which on Windows sits beneath the user profile and +// so contains a space whenever the account name does. Quote each argument +// that needs it; `cmd.exe` (and the CRT parser behind it) strips the quotes +// back off, so the child sees exactly the argument we passed. +const WIN_SHELL = process.platform === "win32"; +function shellArgs(args) { + if (!WIN_SHELL) return args; + return args.map((arg) => + /[\s&|<>^"]/.test(arg) ? `"${arg.replace(/"/g, '""')}"` : arg, + ); +} + /** Run a command to completion, inheriting stdio. Returns the exit status. */ function runInherit(command, args, cwd = repoRoot) { - const r = spawnSync(command, args, { + const r = spawnSync(command, shellArgs(args), { cwd, stdio: "inherit", - shell: process.platform === "win32", + shell: WIN_SHELL, }); return r.status; } @@ -136,10 +152,17 @@ if (runInherit("npm", ["run", "build"]) !== 0) { step("packing the publishable tarball (npm pack)..."); const pack = spawnSync( "npm", - ["pack", "--json", "--ignore-scripts", "--pack-destination", tmpdir()], // 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" }, + // same idiom as runInherit/runBin. `shellArgs` quotes `tmpdir()`, which the + // shell would otherwise split on a space in the user profile path. + shellArgs([ + "pack", + "--json", + "--ignore-scripts", + "--pack-destination", + tmpdir(), + ]), + { cwd: repoRoot, encoding: "utf8", shell: WIN_SHELL }, ); if (pack.status !== 0) { fail(`\`npm pack\` failed:\n${pack.stderr || pack.stdout}`); @@ -253,11 +276,13 @@ try { /** Run the installed bin. Returns { status, output }. */ const runBin = (args, extraEnv = {}) => { - const r = spawnSync(bin, args, { + // `bin` itself is quoted too: it lives under the throwaway consumer's + // `tmpdir()` path, and with a shell the command is joined with the argv. + const r = spawnSync(shellArgs([bin])[0], shellArgs(args), { cwd: work, encoding: "utf8", env: { ...process.env, ...extraEnv }, - shell: process.platform === "win32", + shell: WIN_SHELL, }); return { status: r.status, @@ -378,7 +403,8 @@ async function verifyWeb(bin, cwd) { const host = "127.0.0.1"; const port = process.env.PACK_VERIFY_WEB_PORT ?? "6399"; const token = "pack-verify-token"; - const child = spawn(bin, ["--web"], { + // Quoted for the same reason as runBin: `bin` sits under a `tmpdir()` path. + const child = spawn(shellArgs([bin])[0], ["--web"], { cwd, env: { ...process.env, @@ -388,7 +414,7 @@ async function verifyWeb(bin, cwd) { MCP_AUTO_OPEN_ENABLED: "false", }, stdio: ["ignore", "inherit", "inherit"], - shell: process.platform === "win32", + shell: WIN_SHELL, }); // Expose the child so fail() can kill it if a check below exits the process // (process.exit skips the `finally { stop() }`). From c93552139cff38cb668270502b0d72a4a65c5222 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 17 Aug 2026 00:18:40 -0400 Subject: [PATCH 4/4] fix(scripts): keep the resolver local, complete the cmd.exe quoting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 3, all three real: - `require.resolve.paths()` also appends Node's GLOBAL FOLDERS ($HOME/.node_modules, $HOME/.node_libraries, $PREFIX/lib/node) and any NODE_PATH entries, so round 2's lookup could satisfy a missing repo install from a GLOBALLY installed typescript/vite — measuring the programs with the wrong compiler instead of producing the actionable "run npm install" this helper promises. Filter to the node_modules directories on fromDir's own ancestor chain. - The quoting predicate missed characters cmd.exe still parses, notably parentheses (`C:\Temp(1)`), and quoting cannot suppress `%VAR%` at all — expansion happens before quotes are processed. Extracted to lib/win-shell-args.mjs with a broad predicate and a hard error on `%` rather than a silently different path. `platform` is a parameter so the Windows behavior is exercised on the POSIX machines that run the suite. - AGENTS.md claimed pack-and-verify spawns via `process.execPath`; it does not, and cannot — its children are `npm` and the installed `.bin` shim. State the exception and why. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN Signed-off-by: cliffhall --- AGENTS.md | 13 +++--- scripts/lib/resolve-node-bin.mjs | 33 ++++++++++++++- scripts/lib/resolve-node-bin.test.mjs | 49 +++++++++++++++++++++- scripts/lib/win-shell-args.mjs | 58 +++++++++++++++++++++++++++ scripts/lib/win-shell-args.test.mjs | 54 +++++++++++++++++++++++++ scripts/pack-and-verify.mjs | 21 ++++------ 6 files changed, 207 insertions(+), 21 deletions(-) create mode 100644 scripts/lib/win-shell-args.mjs create mode 100644 scripts/lib/win-shell-args.test.mjs diff --git a/AGENTS.md b/AGENTS.md index 4d901292a..74b403d3f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -129,11 +129,14 @@ v2/main/ │ # (tsc-program.mjs is the `tsc --listFilesOnly` │ # measurement both coverage guards read a program │ # through — #1965; resolve-node-bin.mjs resolves a -│ # package's bin through its own package.json so the -│ # verify/smoke/pack scripts spawn it with -│ # `process.execPath` instead of the `npx`/`npm` -│ # `.cmd` shims Node refuses to spawn shell-free on -│ # Windows — #1939). Prettier-gated via +│ # package's bin through its own package.json, so the +│ # verify/smoke scripts spawn it with `process.execPath` +│ # rather than the `npx` `.cmd` shim Node refuses to +│ # spawn shell-free on Windows. pack-and-verify.mjs is +│ # the exception — its children are `npm` itself and the +│ # installed `.bin` shim, neither resolvable that way, so +│ # it keeps a Windows shell and quotes its generated +│ # paths via win-shell-args.mjs — #1939). Prettier-gated via │ # `format:check:scripts`; its own pure parsers are │ # unit-tested by `npm run test:scripts` (node --test). ├── specification/ # Build specification diff --git a/scripts/lib/resolve-node-bin.mjs b/scripts/lib/resolve-node-bin.mjs index 33d223aed..8caf9f046 100644 --- a/scripts/lib/resolve-node-bin.mjs +++ b/scripts/lib/resolve-node-bin.mjs @@ -69,17 +69,48 @@ export function resolveNodeBin(pkg, binName, fromDir) { * installed and perfectly spawnable — a false "not installed" that would send * someone to `npm install` for a package already on disk. `resolve.paths()` * gives the search dirs without consulting `exports` at all. + * + * Its list is then filtered to the `node_modules` directories on `fromDir`'s + * own ancestor chain, because `resolve.paths()` also appends Node's GLOBAL + * FOLDERS (`$HOME/.node_modules`, `$HOME/.node_libraries`, `$PREFIX/lib/node`) + * and any `NODE_PATH` entries. Accepting those would quietly break the + * guarantee this module exists to keep — that the spawned tsc/vite is the + * REPO-PINNED one, exactly as `npx --no-install` promised. A missing repo + * install would then find a globally installed TypeScript and measure the + * programs with the wrong compiler, instead of failing with the actionable + * "run npm install" this helper is supposed to produce. */ function resolveManifest(pkg, fromDir) { const require = createRequire(path.join(fromDir, "package.json")); // `resolve.paths` returns null only for builtins, which have no bin to find. - for (const dir of require.resolve.paths(pkg) ?? []) { + for (const dir of localSearchDirs( + require.resolve.paths(pkg) ?? [], + fromDir, + )) { const candidate = path.join(dir, ...pkg.split("/"), "package.json"); if (existsSync(candidate)) return candidate; } throw new Error(`cannot find ${pkg} from ${fromDir}`); } +/** + * The subset of `resolve.paths()` that is a `node_modules` directory sitting + * directly under `fromDir` or one of its ancestors — i.e. the local walk, with + * Node's global folders and `NODE_PATH` dropped. Exported for its own test. + */ +export function localSearchDirs(dirs, fromDir) { + const ancestors = new Set(); + for (let dir = path.resolve(fromDir); ; dir = path.dirname(dir)) { + ancestors.add(dir); + if (dir === path.dirname(dir)) break; + } + return dirs.filter( + (dir) => + path.basename(dir) === "node_modules" && + ancestors.has(path.dirname(path.resolve(dir))), + ); +} + /** `@scope/name` → `name`; an unscoped name is returned unchanged. */ function unscopedName(name) { return name.startsWith("@") ? name.slice(name.indexOf("/") + 1) : name; diff --git a/scripts/lib/resolve-node-bin.test.mjs b/scripts/lib/resolve-node-bin.test.mjs index e62a72ed1..febcc749e 100644 --- a/scripts/lib/resolve-node-bin.test.mjs +++ b/scripts/lib/resolve-node-bin.test.mjs @@ -17,7 +17,7 @@ import { import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { resolveNodeBin } from "./resolve-node-bin.mjs"; +import { localSearchDirs, resolveNodeBin } from "./resolve-node-bin.mjs"; const repoRoot = path.resolve( path.dirname(fileURLToPath(import.meta.url)), @@ -148,6 +148,53 @@ test("resolves a package whose `exports` hides ./package.json", () => { } }); +test("localSearchDirs keeps only node_modules on fromDir's ancestor chain", () => { + // `require.resolve.paths()` appends Node's GLOBAL FOLDERS and any NODE_PATH + // entries. Honouring those would break the guarantee this module exists for + // — that the spawned tsc/vite is the REPO-PINNED one — by letting a globally + // installed TypeScript stand in for a missing repo install, measuring the + // programs with the wrong compiler instead of failing actionably. + const from = path.join(path.sep, "repo", "clients", "web"); + const kept = [ + path.join(from, "node_modules"), + path.join(path.sep, "repo", "clients", "node_modules"), + path.join(path.sep, "repo", "node_modules"), + path.join(path.sep, "node_modules"), + ]; + const dropped = [ + path.join(path.sep, "home", "dev", ".node_modules"), + path.join(path.sep, "home", "dev", ".node_libraries"), + path.join(path.sep, "usr", "local", "lib", "node"), + // A NODE_PATH entry off the chain — a real node_modules, just not ours. + path.join(path.sep, "opt", "shared", "node_modules"), + ]; + assert.deepEqual(localSearchDirs([...kept, ...dropped], from), kept); +}); + +test("a package visible only outside the ancestor chain is not resolved", () => { + // The end-to-end form of the above: `ghostpkg` exists on disk, but under a + // sibling tree rather than an ancestor of `fromDir`. + const outside = fixtureDir( + { name: "elsewherepkg", bin: { elsewhere: "bin/e.js" } }, + { createBinFile: true }, + ); + const consumer = mkdtempSync(path.join(tmpdir(), "resolve-node-bin-from-")); + try { + writeFileSync( + path.join(consumer, "package.json"), + JSON.stringify({ name: "consumer" }), + "utf8", + ); + assert.throws( + () => resolveNodeBin("elsewherepkg", "elsewhere", consumer), + /cannot find elsewherepkg/, + ); + } finally { + rmSync(outside, { recursive: true, force: true }); + rmSync(consumer, { recursive: true, force: true }); + } +}); + test("accepts a scoped package's string-form bin under its unscoped name", () => { const dir = fixtureDir( { name: "@scope/tool", bin: "bin/tool.js" }, diff --git a/scripts/lib/win-shell-args.mjs b/scripts/lib/win-shell-args.mjs new file mode 100644 index 000000000..2e653bdf6 --- /dev/null +++ b/scripts/lib/win-shell-args.mjs @@ -0,0 +1,58 @@ +// Quoting for the few spawns that still need a Windows shell (#1939). +// +// Most call sites avoid the shell entirely — `resolve-node-bin.mjs` finds a +// package's JS entry and runs it with `process.execPath`. That is not available +// for `npm`/`npx` themselves: they are not resolvable from `node_modules`, and +// locating npm's own CLI relative to `process.execPath` differs across system, +// nvm, and volta installs — trading a quoting bug for a resolution bug. So the +// `npm` calls keep `shell: true`, which on Windows is the ONLY way to start the +// `.cmd` shim at all (Node has refused shell-free `.cmd`/`.bat` spawns since the +// CVE-2024-27980 hardening). +// +// With a shell, Node hands `cmd.exe` one space-joined string rather than an argv +// array, so any argument holding a space or a `cmd.exe` metacharacter is +// re-parsed as syntax. That is not hypothetical for these callers: every path +// they pass is generated under `tmpdir()`, which on Windows sits beneath the +// user profile — `C:\Users\First Last\AppData\Local\Temp` splits in two, and a +// profile name like `C:\Temp(1)` is read as grouping syntax. + +/** + * `cmd.exe` characters that make an argument parse as syntax rather than text. + * Deliberately broad — parentheses group, `^` escapes, `!` expands under + * delayed expansion, and a bare space splits. Anything not plainly inert is + * quoted; over-quoting is free, since `cmd.exe` and the CRT argument parser + * behind it both strip the quotes back off before the child sees them. + */ +const NEEDS_QUOTING = /[\s&|<>^()!"'`,;=[\]]/; + +/** + * A single argument, safe to pass through `cmd.exe`. + * + * Throws on `%`, which quoting genuinely cannot handle: `cmd.exe` expands + * `%NAME%` *before* it processes quotes, so `"%TEMP%"` is rewritten inside the + * quotes and the child receives a different path than we passed. There is no + * escape for it outside a batch file. A `%` in one of these generated temp + * paths is far-fetched, but silently running against the wrong directory is the + * exact class of failure this work exists to remove — so say so instead. + */ +export function quoteWinArg(arg) { + if (arg.includes("%")) { + throw new Error( + `cannot safely pass an argument containing "%" through cmd.exe (it is ` + + `expanded before quotes are processed): ${arg}`, + ); + } + if (!NEEDS_QUOTING.test(arg)) return arg; + // A literal `"` inside a quoted string is doubled, per the CRT parser. + return `"${arg.replace(/"/g, '""')}"`; +} + +/** + * `args`, quoted for `cmd.exe` when spawning with `shell: true` on Windows and + * returned untouched everywhere else — POSIX shells are not in play here, since + * these call sites only set `shell` on win32. + */ +export function winShellArgs(args, platform = process.platform) { + if (platform !== "win32") return args; + return args.map(quoteWinArg); +} diff --git a/scripts/lib/win-shell-args.test.mjs b/scripts/lib/win-shell-args.test.mjs new file mode 100644 index 000000000..7a1a40fa2 --- /dev/null +++ b/scripts/lib/win-shell-args.test.mjs @@ -0,0 +1,54 @@ +// Tests for `win-shell-args.mjs` (#1939). The platform is a parameter rather +// than a read of `process.platform`, so the Windows behaviour is exercised on +// the Linux/macOS machines that actually run this suite — otherwise every +// assertion below would be skipped exactly where the bug lives. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { quoteWinArg, winShellArgs } from "./win-shell-args.mjs"; + +test("passes args through untouched off Windows", () => { + const args = ["pack", "--pack-destination", "/tmp/dir with spaces"]; + assert.deepEqual(winShellArgs(args, "linux"), args); + assert.deepEqual(winShellArgs(args, "darwin"), args); +}); + +test("quotes a path with a space (the user-profile tmpdir case)", () => { + assert.equal( + quoteWinArg("C:\\Users\\First Last\\AppData\\Local\\Temp"), + '"C:\\Users\\First Last\\AppData\\Local\\Temp"', + ); +}); + +test("quotes cmd.exe metacharacters, parentheses included", () => { + // Parentheses group in cmd.exe, so `C:\Temp(1)` is syntax, not a path — the + // case a space-only predicate misses. + for (const meta of ["(", ")", "&", "|", "<", ">", "^", "!", ";", ",", "="]) { + const arg = `C:\\Temp${meta}1`; + assert.equal(quoteWinArg(arg), `"${arg}"`, `unquoted: ${arg}`); + } +}); + +test("leaves an inert argument unquoted", () => { + for (const arg of ["pack", "--json", "C:\\Users\\dev\\AppData\\Temp"]) { + assert.equal(quoteWinArg(arg), arg); + } +}); + +test("doubles an embedded quote", () => { + assert.equal(quoteWinArg('a"b c'), '"a""b c"'); +}); + +test("throws on `%` rather than silently mis-expanding it", () => { + // cmd.exe expands %NAME% before it processes quotes, so quoting cannot save + // this one — the child would receive a different path than we passed. + assert.throws(() => quoteWinArg("C:\\%TEMP%\\pack"), /cannot safely pass/); + assert.throws(() => winShellArgs(["ok", "%X%"], "win32"), /cannot safely/); +}); + +test("winShellArgs maps every element on win32", () => { + assert.deepEqual( + winShellArgs(["pack", "--pack-destination", "C:\\a b\\c"], "win32"), + ["pack", "--pack-destination", '"C:\\a b\\c"'], + ); +}); diff --git a/scripts/pack-and-verify.mjs b/scripts/pack-and-verify.mjs index 7a0deedfe..1a1f8f2a1 100644 --- a/scripts/pack-and-verify.mjs +++ b/scripts/pack-and-verify.mjs @@ -44,6 +44,7 @@ import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { setTimeout as delay } from "node:timers/promises"; import { hasExited, removeSafe, stopChild } from "./lib/child-cleanup.mjs"; +import { winShellArgs } from "./lib/win-shell-args.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); const testServer = join( @@ -98,21 +99,13 @@ function step(message) { console.log(`\npack:verify — ${message}`); } -// Windows needs `shell: true` to start the `npm`/`npx` `.cmd` shims at all -// (#1939), and with a shell Node hands the argv to `cmd.exe` as ONE -// space-joined string — so an argument holding a path with a space is split -// into two. That is not hypothetical here: every generated path in this file -// lives under `tmpdir()`, which on Windows sits beneath the user profile and -// so contains a space whenever the account name does. Quote each argument -// that needs it; `cmd.exe` (and the CRT parser behind it) strips the quotes -// back off, so the child sees exactly the argument we passed. +// This script keeps `shell: true` on Windows because its children are `npm` and +// `npx` — the two commands `resolve-node-bin.mjs` deliberately cannot replace +// (see the note at the top of `lib/win-shell-args.mjs`) — plus the installed +// `.bin` shim. The shell then re-parses the argv, so every generated path we +// pass has to be quoted for `cmd.exe`. const WIN_SHELL = process.platform === "win32"; -function shellArgs(args) { - if (!WIN_SHELL) return args; - return args.map((arg) => - /[\s&|<>^"]/.test(arg) ? `"${arg.replace(/"/g, '""')}"` : arg, - ); -} +const shellArgs = (args) => winShellArgs(args); /** Run a command to completion, inheriting stdio. Returns the exit status. */ function runInherit(command, args, cwd = repoRoot) {