diff --git a/AGENTS.md b/AGENTS.md index 4ba943ec..4d901292 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 e3330d41..502fe076 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 e631d3f2..33d223ae 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 6ec1e605..e62a72ed 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 2cd4494d..7a0deedf 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() }`).