fix(scripts): keep the resolver local, complete the cmd.exe quoting

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN
Signed-off-by: cliffhall <cliff@futurescale.com>

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;

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" },

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);
+}

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"'],
+  );
+});

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) {