From 4b143ce043bf73ba6c1d62abe1201008be9d0d06 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 23:43:24 -0400 Subject: [PATCH] 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 528611c8..e631d3f2 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 cac917e2..6ec1e605 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 25ee7616..7f3fc79e 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}`;