From 4351037d92ba9b54110f58f2a12aad74448de005 Mon Sep 17 00:00:00 2001 From: ianwremmel-ai-agent Date: Mon, 7 Sep 2026 21:08:32 +0000 Subject: [PATCH 1/7] chore: open PR [skip ci] From 667b515e8fa83768d6855f0c0395918ce0ec3c22 Mon Sep 17 00:00:00 2001 From: ianwremmel-ai-agent Date: Mon, 7 Sep 2026 21:21:50 +0000 Subject: [PATCH 2/7] ci: add a verify-pack job that loads every exports target from the tarball publint and attw read the exports map statically, so a condition that resolves to a file which is present but does not execute passes both. scripts/verify-pack.mjs packs each of the 32 packages under packages/@code-like-a-carpenter, extracts the tarball, and require()s the require target and import()s the import target of every exports entry out of the extracted directory. types and bin targets get an existence check. Loading from the tarball rather than the workspace is the point: a workspace symlink resolves files npm pack may not have included, which is how cli-core and cli-plugin-example came to declare a bin pointing at a cli.mjs that does not exist. npm pack needs a version, which these packages do not carry until multi-semantic-release supplies one at publish time, so each package is staged outside the workspace with a synthetic version before packing. Each extracted package gets a node_modules holding its declared dependencies, so a sibling resolves to its own extracted tarball rather than to the workspace source. Anything undeclared falls through to the workspace's node_modules; policing dependency declarations is tool-deps' job. --- .github/workflows/push.yml | 12 + scripts/verify-pack.mjs | 473 +++++++++++++++++++++++++++++++++++++ 2 files changed, 485 insertions(+) create mode 100755 scripts/verify-pack.mjs diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 92fe9726..30b7fdee 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -82,6 +82,7 @@ jobs: - nopush - test-integration - test-unit + - verify-pack runs-on: ubuntu-latest # This job gets exponentially slower based on the number of packages that # need to be release, so 60 is not unreasonable. @@ -186,3 +187,14 @@ jobs: with: token: ${{ secrets.CODECOV_TOKEN }} directory: 'reports/coverage' + + verify-pack: + needs: build + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + - uses: './.github/actions/setup' + - run: ./scripts/verify-pack.mjs diff --git a/scripts/verify-pack.mjs b/scripts/verify-pack.mjs new file mode 100755 index 00000000..3d959acb --- /dev/null +++ b/scripts/verify-pack.mjs @@ -0,0 +1,473 @@ +#!/usr/bin/env node + +// Packs every publishable package, extracts the tarball, and loads every target +// in its `exports` map out of the extracted directory. +// +// `publint` and `attw` read the `exports` map statically, so a condition that +// resolves to a file which is present but does not execute passes both. Only +// loading the file catches that. Loading from the extracted tarball rather than +// the workspace is the other half: a workspace symlink resolves files `npm +// pack` may not have included. +// +// Each extracted package gets a `node_modules` holding its declared +// dependencies, so a sibling package resolves to *its* extracted tarball and a +// third-party dependency resolves to the version the package asked for rather +// than whatever the workspace root happens to hoist. Anything undeclared falls +// through to the workspace's own `node_modules`; policing dependency +// declarations is `tool-deps`' job, not this one's. + +import {spawn} from 'node:child_process'; +import fs from 'node:fs/promises'; +import {createRequire} from 'node:module'; +import os from 'node:os'; +import path from 'node:path'; +import process from 'node:process'; +import {pathToFileURL} from 'node:url'; + +/** @typedef {{subpath: string, conditions: string[], target: string}} Target */ +/** @typedef {{name: string, dir: string, source: string, manifest: Record}} Package */ + +const PACKAGES_DIR = 'packages/@code-like-a-carpenter'; + +// Packages carry no `version` — multi-semantic-release supplies one at publish +// time — and `npm pack` refuses to run without one. +const SYNTHETIC_VERSION = '0.0.0-verify-pack'; + +// `@code-like-a-carpenter/cli`'s `.` export is the CLI itself: it calls +// `main()` at module scope, and its `bin` is a one-line wrapper that imports +// it. Loading it runs the CLI and exits, so it gets an existence check. +const PROGRAM_ENTRY_POINTS = new Set(['@code-like-a-carpenter/cli']); + +/** + * @param {string} command + * @param {string[]} args + * @param {string} cwd + * @returns {Promise<{code: number, stdout: string, stderr: string}>} + */ +function run(command, args, cwd) { + return new Promise((resolve, reject) => { + const child = spawn(command, args, { + cwd, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let stdout = ''; + let stderr = ''; + child.stdout.on('data', (chunk) => (stdout += chunk)); + child.stderr.on('data', (chunk) => (stderr += chunk)); + child.on('error', reject); + child.on('close', (code) => resolve({code: code ?? 1, stderr, stdout})); + }); +} + +/** + * Every string leaf of an `exports` subpath value, tagged with the conditions + * it sits under. + * + * @param {string} subpath + * @param {unknown} value + * @param {string[]} conditions + * @returns {Target[]} + */ +function collectTargets(subpath, value, conditions = []) { + if (typeof value === 'string') { + return [{conditions, subpath, target: value}]; + } + if (value && typeof value === 'object' && !Array.isArray(value)) { + return Object.entries(value).flatMap(([condition, nested]) => + collectTargets(subpath, nested, [...conditions, condition]) + ); + } + // `null` blocks a subpath, and the array fallback form is not used here; + // neither has a file to check. + return []; +} + +/** + * @param {unknown} bin + * @returns {Target[]} + */ +function binTargets(bin) { + if (typeof bin === 'string') { + return [{conditions: ['bin'], subpath: 'bin', target: bin}]; + } + if (bin && typeof bin === 'object') { + return Object.entries(bin) + .filter(([, target]) => typeof target === 'string') + .map(([name, target]) => ({ + conditions: ['bin'], + subpath: `bin.${name}`, + target, + })); + } + return []; +} + +/** + * @param {Record} manifest + * @returns {Target[]} + */ +function targetsOf(manifest) { + const {exports: exportsMap} = manifest; + if (exportsMap === undefined) { + throw new Error('package has no "exports" map'); + } + + /** @type {Target[]} */ + const targets = + typeof exportsMap === 'string' + ? collectTargets('.', exportsMap) + : Object.entries(exportsMap).flatMap(([key, value]) => + // A key that does not start with "." is a condition on ".". + key.startsWith('.') + ? collectTargets(key, value) + : collectTargets('.', value, [key]) + ); + + if (typeof manifest.types === 'string') { + targets.push({ + conditions: ['types'], + subpath: 'types', + target: manifest.types, + }); + } + + return [...targets, ...binTargets(manifest.bin)]; +} + +/** + * `types` targets are declarations and `bin` targets are programs; neither is + * loadable as a module, so both get an existence check. + * + * @param {Target} target + * @param {string} packageName + * @returns {'import' | 'require' | 'exists'} + */ +function checkFor({conditions, subpath}, packageName) { + if (conditions.includes('types') || conditions.includes('bin')) { + return 'exists'; + } + if (subpath === '.' && PROGRAM_ENTRY_POINTS.has(packageName)) { + return 'exists'; + } + if (conditions.includes('require')) { + return 'require'; + } + if (conditions.includes('import')) { + return 'import'; + } + return 'exists'; +} + +/** @param {Target} target */ +function describe({conditions, subpath, target}) { + const suffix = conditions.length ? ` (${conditions.join('.')})` : ''; + return `${subpath}${suffix} -> ${target}`; +} + +/** + * Loads every target of one extracted package. Runs in a child process so that + * an entry point which kills the process is reported rather than taking the + * whole run down with it. + * + * @param {string} packageDir + * @returns {Promise} + */ +async function loadPackage(packageDir) { + const manifest = JSON.parse( + await fs.readFile(path.join(packageDir, 'package.json'), 'utf8') + ); + const require = createRequire(import.meta.url); + + /** @type {string[]} */ + const failures = []; + + for (const target of targetsOf(manifest)) { + const resolved = path.resolve(packageDir, target.target); + const label = describe(target); + + try { + await fs.access(resolved); + } catch { + failures.push(`${label}: missing from the tarball`); + continue; + } + + const check = checkFor(target, manifest.name); + try { + if (check === 'require') { + require(resolved); + } else if (check === 'import') { + await import(pathToFileURL(resolved).href); + } + } catch (err) { + failures.push(`${label}: ${check}() threw: ${err}`); + } + } + + for (const failure of failures) { + process.stderr.write(`${failure}\n`); + } + return failures.length === 0 ? 0 : 1; +} + +/** + * The directory node would resolve `dependency` to from `fromDir`, found the + * way node finds it: by walking up looking for `node_modules/`. + * `require.resolve` cannot stand in here because a package whose `exports` map + * omits `./package.json` is unresolvable by specifier. + * + * @param {string} fromDir + * @param {string} dependency + * @returns {Promise} + */ +async function resolvePackageDir(fromDir, dependency) { + let dir = fromDir; + for (;;) { + const candidate = path.join(dir, 'node_modules', dependency); + try { + await fs.access(path.join(candidate, 'package.json')); + return await fs.realpath(candidate); + } catch { + // keep walking + } + const parent = path.dirname(dir); + if (parent === dir) { + return null; + } + dir = parent; + } +} + +/** + * @param {Package} pkg + * @param {string} stageRoot + * @returns {Promise} + */ +async function packAndExtract(pkg, stageRoot) { + // Staging happens outside the workspace so that `npm pack` sees a plain + // package rather than a workspace member. + const stage = path.join(stageRoot, path.basename(pkg.source)); + await fs.cp(pkg.source, stage, { + filter: (src) => path.basename(src) !== 'node_modules', + recursive: true, + }); + await fs.writeFile( + path.join(stage, 'package.json'), + `${JSON.stringify({...pkg.manifest, version: SYNTHETIC_VERSION}, null, 2)}\n` + ); + + const packed = await run( + 'npm', + ['pack', '--ignore-scripts', '--json', '--pack-destination', stageRoot], + stage + ); + if (packed.code !== 0) { + throw new Error(`npm pack failed:\n${packed.stderr}`); + } + const [{filename}] = JSON.parse(packed.stdout); + + await fs.mkdir(pkg.dir, {recursive: true}); + const extracted = await run( + 'tar', + [ + '--extract', + '--gzip', + '--strip-components=1', + '--file', + path.join(stageRoot, filename), + '--directory', + pkg.dir, + ], + stageRoot + ); + if (extracted.code !== 0) { + throw new Error(`tar failed:\n${extracted.stderr}`); + } +} + +/** + * @param {Package} pkg + * @param {Map} packages + * @returns {Promise} dependencies that could not be resolved + */ +async function linkDependencies(pkg, packages) { + const dependencies = Object.keys({ + ...pkg.manifest.dependencies, + ...pkg.manifest.optionalDependencies, + }); + + /** @type {string[]} */ + const unresolved = []; + + for (const dependency of dependencies) { + const sibling = packages.get(dependency); + const target = sibling + ? sibling.dir + : await resolvePackageDir(pkg.source, dependency); + + if (!target) { + unresolved.push(dependency); + continue; + } + + const link = path.join(pkg.dir, 'node_modules', dependency); + await fs.mkdir(path.dirname(link), {recursive: true}); + await fs.symlink(target, link, 'dir'); + } + + return unresolved; +} + +/** + * Every publishable package under {@link PACKAGES_DIR}, keyed by package name. + * + * @param {string} packagesRoot + * @param {string} extractRoot + * @returns {Promise>} + */ +async function findPackages(packagesRoot, extractRoot) { + const entries = await fs.readdir(packagesRoot, {withFileTypes: true}); + + /** @type {Map} */ + const packages = new Map(); + + for (const entry of entries.filter((e) => e.isDirectory()).sort()) { + const source = path.join(packagesRoot, entry.name); + const manifest = JSON.parse( + await fs.readFile(path.join(source, 'package.json'), 'utf8') + ); + if (!manifest.private) { + packages.set(manifest.name, { + dir: path.join(extractRoot, manifest.name), + manifest, + name: manifest.name, + source, + }); + } + } + + return packages; +} + +/** + * @param {Map} packages + * @param {string} stageRoot + * @returns {Promise>} failures, keyed by package name + */ +async function prepare(packages, stageRoot) { + /** @type {Map} */ + const failures = new Map(); + + for (const pkg of packages.values()) { + try { + await packAndExtract(pkg, stageRoot); + } catch (err) { + failures.set(pkg.name, [String(err)]); + } + } + + for (const pkg of packages.values()) { + if (failures.has(pkg.name)) { + continue; + } + const unresolved = await linkDependencies(pkg, packages); + if (unresolved.length) { + failures.set(pkg.name, [ + `dependencies are not installed in the workspace: ${unresolved.join(', ')}`, + ]); + } + } + + return failures; +} + +/** + * @param {Map} packages + * @param {Map} failures + * @returns {Promise} + */ +async function loadAll(packages, failures) { + for (const pkg of packages.values()) { + if (failures.has(pkg.name)) { + process.stdout.write(`FAIL ${pkg.name}\n`); + continue; + } + + const result = await run( + process.execPath, + [import.meta.filename, '--load', pkg.dir], + process.cwd() + ); + + if (result.code === 0) { + process.stdout.write(`ok ${pkg.name}\n`); + } else { + const output = `${result.stderr}${result.stdout}`.trimEnd(); + failures.set( + pkg.name, + output ? output.split('\n') : ['loading exited non-zero'] + ); + process.stdout.write(`FAIL ${pkg.name}\n`); + } + } +} + +/** + * @param {Map} failures + * @param {number} total + * @returns {void} + */ +function report(failures, total) { + if (failures.size === 0) { + process.stdout.write(`\nAll ${total} packages load from their tarball\n`); + return; + } + + process.stdout.write('\n'); + for (const [name, lines] of failures) { + process.stdout.write(`${name}\n`); + for (const line of lines) { + process.stdout.write(` ${line}\n`); + } + } + + const prefix = process.env.GITHUB_ACTIONS ? '::error::' : ''; + process.stdout.write( + `${prefix}${failures.size} of ${total} packages do not load from their tarball: ${[...failures.keys()].join(', ')}\n` + ); + process.exitCode = 1; +} + +async function main() { + const workspaceRoot = process.cwd(); + const workRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'verify-pack-')); + const stageRoot = path.join(workRoot, 'stage'); + await fs.mkdir(stageRoot, {recursive: true}); + // Puts the workspace's node_modules on the resolution path of every extracted + // package, one directory above the extract root, so that an undeclared + // dependency resolves the way it does in the workspace. + await fs.symlink( + path.join(workspaceRoot, 'node_modules'), + path.join(workRoot, 'node_modules'), + 'dir' + ); + + const packages = await findPackages( + path.join(workspaceRoot, PACKAGES_DIR), + path.join(workRoot, 'extracted') + ); + + try { + const failures = await prepare(packages, stageRoot); + await loadAll(packages, failures); + report(failures, packages.size); + } finally { + await fs.rm(workRoot, {force: true, recursive: true}); + } +} + +if (process.argv[2] === '--load') { + process.exitCode = await loadPackage(process.argv[3]); +} else { + await main(); +} From 9e6f4ba7160f5475beee83953cac372aa5a24d93 Mon Sep 17 00:00:00 2001 From: ianwremmel-ai-agent Date: Mon, 7 Sep 2026 21:37:09 +0000 Subject: [PATCH 3/7] ci(verify-pack): load each target in its own process and close the silent passes Two adversarial review passes found four ways the check reported ok for a package it had not actually verified. A target that calls process.exit() during evaluation ended the child, so every target queued behind it in the same process was skipped and the zero exit code read as success. A CLI whose "." export runs itself is exactly that shape, which is why the previous version needed an allowlist to skip @code-like-a-carpenter/cli. Each loadable target now gets its own child, and success is a marker the child writes once the module has evaluated rather than the exit code. A module that exits while evaluating still executed, so the marker is written from an exit handler; only a module that throws suppresses it. The allowlist is gone and cli's entry point is now really loaded. checkFor fell through to an existence check for any condition that was not literally require or import, so a flat {types, default} map -- or a bare string target -- got the publint-grade check this script exists to improve on. It now picks a loader from the extension and the package's type field. Subpath patterns are reported as unsupported instead of failing as a missing file. A sibling that failed to pack still got a symlink, and node walks past a dangling link to the workspace's node_modules, where the scope is symlinked back to the unpacked source. The dependent package then reported ok having been verified against the source tree. Dependents of a failed package now fail, and a link is only created when its target exists. Nothing bounded the children. A module that left the event loop alive hung the run to the job timeout -- CI already logged an orphaned node process. Loading now stops the child as soon as the module evaluates, and every spawn has a 60s kill. Also: enumerate packages/*/* rather than one scope, skip directories with no manifest, stop dependency resolution at the workspace root, do not treat optionalDependencies as mandatory, catch errors from linking, guard the npm pack JSON parse, decode child output per stream, fix a no-op sort that left package order filesystem-dependent, move setup inside the try that cleans up the temp tree, and annotate each failing target rather than only the summary line. Packing and loading now run concurrently, which takes the full check from 58s to 9s. --- scripts/verify-pack.mjs | 446 +++++++++++++++++++++++++++------------- 1 file changed, 308 insertions(+), 138 deletions(-) diff --git a/scripts/verify-pack.mjs b/scripts/verify-pack.mjs index 3d959acb..c6f642c2 100755 --- a/scripts/verify-pack.mjs +++ b/scripts/verify-pack.mjs @@ -9,15 +9,24 @@ // the workspace is the other half: a workspace symlink resolves files `npm // pack` may not have included. // +// Each loadable target is loaded in its own child process. One process per +// package would let an entry point that calls `process.exit()` — a CLI whose +// `.` export runs itself — report success for every target queued behind it. +// The child writes MARKER once the module has evaluated, and only that marker +// counts as a pass; the exit code alone cannot tell "loaded, then exited" from +// "threw while loading". +// // Each extracted package gets a `node_modules` holding its declared -// dependencies, so a sibling package resolves to *its* extracted tarball and a -// third-party dependency resolves to the version the package asked for rather -// than whatever the workspace root happens to hoist. Anything undeclared falls -// through to the workspace's own `node_modules`; policing dependency -// declarations is `tool-deps`' job, not this one's. +// dependencies, so a sibling resolves to *its* extracted tarball rather than to +// the workspace source. Third-party dependencies resolve exactly where node +// would resolve them from the source package, which is the nested install where +// there is one and the root hoist otherwise. Anything undeclared falls through +// to the workspace's own `node_modules`; policing dependency declarations is +// `tool-deps`' job, not this one's. import {spawn} from 'node:child_process'; -import fs from 'node:fs/promises'; +import fs from 'node:fs'; +import fsp from 'node:fs/promises'; import {createRequire} from 'node:module'; import os from 'node:os'; import path from 'node:path'; @@ -27,22 +36,28 @@ import {pathToFileURL} from 'node:url'; /** @typedef {{subpath: string, conditions: string[], target: string}} Target */ /** @typedef {{name: string, dir: string, source: string, manifest: Record}} Package */ -const PACKAGES_DIR = 'packages/@code-like-a-carpenter'; +const PACKAGES_DIR = 'packages'; // Packages carry no `version` — multi-semantic-release supplies one at publish // time — and `npm pack` refuses to run without one. const SYNTHETIC_VERSION = '0.0.0-verify-pack'; -// `@code-like-a-carpenter/cli`'s `.` export is the CLI itself: it calls -// `main()` at module scope, and its `bin` is a one-line wrapper that imports -// it. Loading it runs the CLI and exits, so it gets an existence check. -const PROGRAM_ENTRY_POINTS = new Set(['@code-like-a-carpenter/cli']); +// Written by the child once the module under test has evaluated. `fs.writeSync` +// rather than `process.stdout.write` because the child writes it from an `exit` +// handler, where an async write to a pipe would be dropped. +const MARKER = '@@verify-pack:evaluated@@'; + +// A module that blocks the event loop cannot be waited out. Without this a +// single bad entry point burns the whole CI job's budget. +const CHILD_TIMEOUT_MS = 60_000; + +const CONCURRENCY = Math.max(1, Math.min(8, os.availableParallelism())); /** * @param {string} command * @param {string[]} args * @param {string} cwd - * @returns {Promise<{code: number, stdout: string, stderr: string}>} + * @returns {Promise<{code: number, stdout: string, stderr: string, timedOut: boolean}>} */ function run(command, args, cwd) { return new Promise((resolve, reject) => { @@ -50,15 +65,53 @@ function run(command, args, cwd) { cwd, stdio: ['ignore', 'pipe', 'pipe'], }); + let stdout = ''; let stderr = ''; + let timedOut = false; + + // Decode per stream rather than per chunk so a multi-byte sequence split + // across chunks is not mangled. + child.stdout.setEncoding('utf8'); + child.stderr.setEncoding('utf8'); child.stdout.on('data', (chunk) => (stdout += chunk)); child.stderr.on('data', (chunk) => (stderr += chunk)); - child.on('error', reject); - child.on('close', (code) => resolve({code: code ?? 1, stderr, stdout})); + + const timer = setTimeout(() => { + timedOut = true; + child.kill('SIGKILL'); + }, CHILD_TIMEOUT_MS); + + child.on('error', (err) => { + clearTimeout(timer); + reject(err); + }); + child.on('close', (code) => { + clearTimeout(timer); + resolve({code: code ?? 1, stderr, stdout, timedOut}); + }); }); } +/** + * @template T + * @param {T[]} items + * @param {(item: T) => Promise} fn + * @returns {Promise} + */ +async function forEachConcurrently(items, fn) { + const queue = [...items]; + const workers = Array.from( + {length: Math.min(CONCURRENCY, queue.length)}, + async () => { + for (let item = queue.shift(); item !== undefined; item = queue.shift()) { + await fn(item); + } + } + ); + await Promise.all(workers); +} + /** * Every string leaf of an `exports` subpath value, tagged with the conditions * it sits under. @@ -135,18 +188,24 @@ function targetsOf(manifest) { } /** - * `types` targets are declarations and `bin` targets are programs; neither is - * loadable as a module, so both get an existence check. + * How to check one target. `types` targets are declarations, `bin` targets are + * programs and JSON is data, so those get an existence check. Everything else + * is loaded, and a target under no condition this function recognises still + * picks a loader from its extension and the package's `type` rather than + * quietly degrading to an existence check. * * @param {Target} target - * @param {string} packageName - * @returns {'import' | 'require' | 'exists'} + * @param {Record} manifest + * @returns {'import' | 'require' | 'exists' | 'unsupported'} */ -function checkFor({conditions, subpath}, packageName) { +function checkFor({conditions, target}, manifest) { + if (target.includes('*')) { + return 'unsupported'; + } if (conditions.includes('types') || conditions.includes('bin')) { return 'exists'; } - if (subpath === '.' && PROGRAM_ENTRY_POINTS.has(packageName)) { + if (target.endsWith('.json')) { return 'exists'; } if (conditions.includes('require')) { @@ -155,7 +214,13 @@ function checkFor({conditions, subpath}, packageName) { if (conditions.includes('import')) { return 'import'; } - return 'exists'; + if (target.endsWith('.cjs')) { + return 'require'; + } + if (target.endsWith('.mjs')) { + return 'import'; + } + return manifest.type === 'module' ? 'import' : 'require'; } /** @param {Target} target */ @@ -165,49 +230,40 @@ function describe({conditions, subpath, target}) { } /** - * Loads every target of one extracted package. Runs in a child process so that - * an entry point which kills the process is reported rather than taking the - * whole run down with it. + * Loads one target and writes {@link MARKER} if it evaluated. A module that + * calls `process.exit()` while evaluating still executed, so the marker is + * written from an exit handler; only a module that throws suppresses it. * - * @param {string} packageDir - * @returns {Promise} + * @param {'import' | 'require'} mode + * @param {string} file + * @returns {Promise} */ -async function loadPackage(packageDir) { - const manifest = JSON.parse( - await fs.readFile(path.join(packageDir, 'package.json'), 'utf8') - ); - const require = createRequire(import.meta.url); - - /** @type {string[]} */ - const failures = []; +async function loadTarget(mode, file) { + let started = false; + let threw = false; - for (const target of targetsOf(manifest)) { - const resolved = path.resolve(packageDir, target.target); - const label = describe(target); - - try { - await fs.access(resolved); - } catch { - failures.push(`${label}: missing from the tarball`); - continue; + process.on('exit', () => { + if (started && !threw) { + fs.writeSync(1, MARKER); } + }); - const check = checkFor(target, manifest.name); - try { - if (check === 'require') { - require(resolved); - } else if (check === 'import') { - await import(pathToFileURL(resolved).href); - } - } catch (err) { - failures.push(`${label}: ${check}() threw: ${err}`); + try { + started = true; + if (mode === 'require') { + createRequire(import.meta.url)(file); + } else { + await import(pathToFileURL(file).href); } + } catch (err) { + threw = true; + process.stderr.write(`${err instanceof Error ? err.stack : err}\n`); + process.exit(1); } - for (const failure of failures) { - process.stderr.write(`${failure}\n`); - } - return failures.length === 0 ? 0 : 1; + // The module loaded. Leaving normally would wait on whatever handles it + // opened, so stop here rather than hanging on a timer or an open socket. + process.exit(0); } /** @@ -217,19 +273,24 @@ async function loadPackage(packageDir) { * omits `./package.json` is unresolvable by specifier. * * @param {string} fromDir + * @param {string} stopDir the workspace root; resolving past it would reach + * modules that are not part of the checkout * @param {string} dependency * @returns {Promise} */ -async function resolvePackageDir(fromDir, dependency) { +async function resolvePackageDir(fromDir, stopDir, dependency) { let dir = fromDir; for (;;) { const candidate = path.join(dir, 'node_modules', dependency); try { - await fs.access(path.join(candidate, 'package.json')); - return await fs.realpath(candidate); + await fsp.access(path.join(candidate, 'package.json')); + return await fsp.realpath(candidate); } catch { // keep walking } + if (dir === stopDir) { + return null; + } const parent = path.dirname(dir); if (parent === dir) { return null; @@ -246,27 +307,36 @@ async function resolvePackageDir(fromDir, dependency) { async function packAndExtract(pkg, stageRoot) { // Staging happens outside the workspace so that `npm pack` sees a plain // package rather than a workspace member. - const stage = path.join(stageRoot, path.basename(pkg.source)); - await fs.cp(pkg.source, stage, { + const stage = path.join(stageRoot, pkg.name.replace(/[@/]/g, '_')); + await fsp.cp(pkg.source, stage, { filter: (src) => path.basename(src) !== 'node_modules', recursive: true, }); - await fs.writeFile( + await fsp.writeFile( path.join(stage, 'package.json'), `${JSON.stringify({...pkg.manifest, version: SYNTHETIC_VERSION}, null, 2)}\n` ); const packed = await run( 'npm', - ['pack', '--ignore-scripts', '--json', '--pack-destination', stageRoot], + ['pack', '--ignore-scripts', '--json', '--pack-destination', stage], stage ); if (packed.code !== 0) { throw new Error(`npm pack failed:\n${packed.stderr}`); } - const [{filename}] = JSON.parse(packed.stdout); - await fs.mkdir(pkg.dir, {recursive: true}); + let filename; + try { + [{filename}] = JSON.parse(packed.stdout); + } catch { + throw new Error(`could not read npm pack output:\n${packed.stdout}`); + } + if (typeof filename !== 'string') { + throw new Error(`npm pack named no tarball:\n${packed.stdout}`); + } + + await fsp.mkdir(pkg.dir, {recursive: true}); const extracted = await run( 'tar', [ @@ -274,11 +344,11 @@ async function packAndExtract(pkg, stageRoot) { '--gzip', '--strip-components=1', '--file', - path.join(stageRoot, filename), + path.join(stage, filename), '--directory', pkg.dir, ], - stageRoot + stage ); if (extracted.code !== 0) { throw new Error(`tar failed:\n${extracted.stderr}`); @@ -288,9 +358,13 @@ async function packAndExtract(pkg, stageRoot) { /** * @param {Package} pkg * @param {Map} packages + * @param {string} workspaceRoot * @returns {Promise} dependencies that could not be resolved */ -async function linkDependencies(pkg, packages) { +async function linkDependencies(pkg, packages, workspaceRoot) { + const optional = new Set( + Object.keys(pkg.manifest.optionalDependencies ?? {}) + ); const dependencies = Object.keys({ ...pkg.manifest.dependencies, ...pkg.manifest.optionalDependencies, @@ -303,16 +377,21 @@ async function linkDependencies(pkg, packages) { const sibling = packages.get(dependency); const target = sibling ? sibling.dir - : await resolvePackageDir(pkg.source, dependency); - - if (!target) { - unresolved.push(dependency); + : await resolvePackageDir(pkg.source, workspaceRoot, dependency); + + // A dangling link is worse than no link: node walks past it and resolves + // the dependency from the workspace instead, which is the unpacked source + // tree this whole check exists to avoid. + if (!target || !fs.existsSync(target)) { + if (!optional.has(dependency)) { + unresolved.push(dependency); + } continue; } const link = path.join(pkg.dir, 'node_modules', dependency); - await fs.mkdir(path.dirname(link), {recursive: true}); - await fs.symlink(target, link, 'dir'); + await fsp.mkdir(path.dirname(link), {recursive: true}); + await fsp.symlink(target, link, 'dir'); } return unresolved; @@ -320,61 +399,101 @@ async function linkDependencies(pkg, packages) { /** * Every publishable package under {@link PACKAGES_DIR}, keyed by package name. + * The workspace globs `packages/*` + '/*', so scopes other than + * `@code-like-a-carpenter` count too. * * @param {string} packagesRoot * @param {string} extractRoot * @returns {Promise>} */ async function findPackages(packagesRoot, extractRoot) { - const entries = await fs.readdir(packagesRoot, {withFileTypes: true}); - /** @type {Map} */ const packages = new Map(); - for (const entry of entries.filter((e) => e.isDirectory()).sort()) { - const source = path.join(packagesRoot, entry.name); - const manifest = JSON.parse( - await fs.readFile(path.join(source, 'package.json'), 'utf8') - ); - if (!manifest.private) { - packages.set(manifest.name, { - dir: path.join(extractRoot, manifest.name), - manifest, - name: manifest.name, - source, - }); + const scopes = await fsp.readdir(packagesRoot, {withFileTypes: true}); + for (const scope of scopes.filter((entry) => entry.isDirectory())) { + const scopeRoot = path.join(packagesRoot, scope.name); + const entries = await fsp.readdir(scopeRoot, {withFileTypes: true}); + + for (const entry of entries.filter((e) => e.isDirectory())) { + const source = path.join(scopeRoot, entry.name); + let manifest; + try { + manifest = JSON.parse( + await fsp.readFile(path.join(source, 'package.json'), 'utf8') + ); + } catch { + // Not a package — a stray build or cache directory. + continue; + } + if (!manifest.private) { + packages.set(manifest.name, { + dir: path.join(extractRoot, manifest.name), + manifest, + name: manifest.name, + source, + }); + } } } - return packages; + return new Map( + [...packages.entries()].sort(([a], [b]) => a.localeCompare(b)) + ); } /** + * Packs, extracts and wires up `node_modules` for every package. + * * @param {Map} packages * @param {string} stageRoot + * @param {string} workspaceRoot * @returns {Promise>} failures, keyed by package name */ -async function prepare(packages, stageRoot) { +async function prepare(packages, stageRoot, workspaceRoot) { /** @type {Map} */ const failures = new Map(); - for (const pkg of packages.values()) { + await forEachConcurrently([...packages.values()], async (pkg) => { try { await packAndExtract(pkg, stageRoot); } catch (err) { failures.set(pkg.name, [String(err)]); } + }); + + // A package whose sibling failed to pack cannot be verified against that + // sibling's tarball, so it fails too rather than silently resolving the + // sibling from the workspace. + for (let changed = true; changed; ) { + changed = false; + for (const pkg of packages.values()) { + if (failures.has(pkg.name)) { + continue; + } + const broken = Object.keys(pkg.manifest.dependencies ?? {}).find( + (dep) => packages.has(dep) && failures.has(dep) + ); + if (broken) { + failures.set(pkg.name, [`depends on ${broken}, which failed to pack`]); + changed = true; + } + } } for (const pkg of packages.values()) { if (failures.has(pkg.name)) { continue; } - const unresolved = await linkDependencies(pkg, packages); - if (unresolved.length) { - failures.set(pkg.name, [ - `dependencies are not installed in the workspace: ${unresolved.join(', ')}`, - ]); + try { + const unresolved = await linkDependencies(pkg, packages, workspaceRoot); + if (unresolved.length) { + failures.set(pkg.name, [ + `dependencies are not installed in the workspace: ${unresolved.join(', ')}`, + ]); + } + } catch (err) { + failures.set(pkg.name, [`could not link dependencies: ${err}`]); } } @@ -382,34 +501,59 @@ async function prepare(packages, stageRoot) { } /** - * @param {Map} packages - * @param {Map} failures - * @returns {Promise} + * @param {Package} pkg + * @returns {Promise} one line per failed target */ -async function loadAll(packages, failures) { - for (const pkg of packages.values()) { - if (failures.has(pkg.name)) { - process.stdout.write(`FAIL ${pkg.name}\n`); +async function checkPackage(pkg) { + /** @type {string[]} */ + const problems = []; + + /** @type {Target[]} */ + let targets; + try { + targets = targetsOf(pkg.manifest); + } catch (err) { + return [String(err)]; + } + + for (const target of targets) { + const label = describe(target); + const check = checkFor(target, pkg.manifest); + + if (check === 'unsupported') { + problems.push(`${label}: subpath patterns are not supported`); + continue; + } + + const file = path.resolve(pkg.dir, target.target); + if (!fs.existsSync(file)) { + problems.push(`${label}: missing from the tarball`); + continue; + } + if (check === 'exists') { continue; } const result = await run( process.execPath, - [import.meta.filename, '--load', pkg.dir], - process.cwd() + [import.meta.filename, '--load', check, file], + pkg.dir ); - if (result.code === 0) { - process.stdout.write(`ok ${pkg.name}\n`); - } else { - const output = `${result.stderr}${result.stdout}`.trimEnd(); - failures.set( - pkg.name, - output ? output.split('\n') : ['loading exited non-zero'] + if (result.timedOut) { + problems.push( + `${label}: ${check}() did not finish within ${CHILD_TIMEOUT_MS / 1000}s` + ); + } else if (!result.stdout.includes(MARKER)) { + const detail = + `${result.stderr}${result.stdout.replaceAll(MARKER, '')}`.trim(); + problems.push( + `${label}: ${check}() failed${detail ? `: ${detail}` : ` with exit code ${result.code}`}` ); - process.stdout.write(`FAIL ${pkg.name}\n`); } } + + return problems; } /** @@ -423,51 +567,77 @@ function report(failures, total) { return; } + const annotate = process.env.GITHUB_ACTIONS ? '::error::' : ''; + process.stdout.write('\n'); for (const [name, lines] of failures) { - process.stdout.write(`${name}\n`); for (const line of lines) { - process.stdout.write(` ${line}\n`); + process.stdout.write(`${annotate}${name}: ${line}\n`); } } - const prefix = process.env.GITHUB_ACTIONS ? '::error::' : ''; process.stdout.write( - `${prefix}${failures.size} of ${total} packages do not load from their tarball: ${[...failures.keys()].join(', ')}\n` + `\n${failures.size} of ${total} packages do not load from their tarball: ${[...failures.keys()].join(', ')}\n` ); process.exitCode = 1; } async function main() { const workspaceRoot = process.cwd(); - const workRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'verify-pack-')); - const stageRoot = path.join(workRoot, 'stage'); - await fs.mkdir(stageRoot, {recursive: true}); - // Puts the workspace's node_modules on the resolution path of every extracted - // package, one directory above the extract root, so that an undeclared - // dependency resolves the way it does in the workspace. - await fs.symlink( - path.join(workspaceRoot, 'node_modules'), - path.join(workRoot, 'node_modules'), - 'dir' - ); - - const packages = await findPackages( - path.join(workspaceRoot, PACKAGES_DIR), - path.join(workRoot, 'extracted') - ); + const workRoot = await fsp.mkdtemp(path.join(os.tmpdir(), 'verify-pack-')); try { - const failures = await prepare(packages, stageRoot); - await loadAll(packages, failures); - report(failures, packages.size); + const stageRoot = path.join(workRoot, 'stage'); + await fsp.mkdir(stageRoot, {recursive: true}); + // Puts the workspace's node_modules on the resolution path of every + // extracted package, one directory above the extract root, so that an + // undeclared dependency resolves the way it does in the workspace. + await fsp.symlink( + path.join(workspaceRoot, 'node_modules'), + path.join(workRoot, 'node_modules'), + 'dir' + ); + + const packages = await findPackages( + path.join(workspaceRoot, PACKAGES_DIR), + path.join(workRoot, 'extracted') + ); + + const failures = await prepare(packages, stageRoot, workspaceRoot); + + await forEachConcurrently( + [...packages.values()].filter((pkg) => !failures.has(pkg.name)), + async (pkg) => { + const problems = await checkPackage(pkg); + if (problems.length) { + failures.set(pkg.name, problems); + } + } + ); + + for (const pkg of packages.values()) { + process.stdout.write( + `${failures.has(pkg.name) ? 'FAIL' : 'ok '} ${pkg.name}\n` + ); + } + + report( + new Map([...failures.entries()].sort(([a], [b]) => a.localeCompare(b))), + packages.size + ); } finally { - await fs.rm(workRoot, {force: true, recursive: true}); + // `fs.rm` unlinks symlinks rather than following them, so this removes the + // links into the workspace without touching what they point at. Do not + // replace it with anything that dereferences. + await fsp.rm(workRoot, {force: true, recursive: true}); } } if (process.argv[2] === '--load') { - process.exitCode = await loadPackage(process.argv[3]); + await loadTarget( + /** @type {'import' | 'require'} */ (process.argv[3]), + process.argv[4] + ); } else { await main(); } From c203b8c05e7ab08b9620339c644a47b47ddff334 Mon Sep 17 00:00:00 2001 From: ianwremmel-ai-agent Date: Mon, 7 Sep 2026 22:25:13 +0000 Subject: [PATCH 4/7] ci(verify-pack): require a clean exit, not just an evaluation marker A cross-model review pass found three paths that reported a broken package as `ok`. The loader wrote its evaluated-marker from an `exit` handler whenever loading had started without throwing, and the parent treated that marker alone as a pass. A module calling `process.exit(1)` while evaluating, or leaving a top-level `await` unsettled (node exits 13), produced the marker and passed. The marker now also requires a zero exit code. A module that evaluates and exits cleanly, including a CLI that runs itself on import, still passes. Fallback arrays in an `exports` map collected no targets at all, so `{".": ["./missing.mjs"]}` passed vacuously. They are reported as unsupported, which fails, rather than skipped. A `package.json` that exists but does not parse hit the same `catch` as a missing one and was dropped from the run, letting the job report that every package it did look at was fine. A malformed manifest now aborts. Staging flattened the scope into the directory name, mapping `@a/b_c` and `@a_b/c` onto one directory. It keeps the scope as a directory now, as the extract root already did. --- scripts/verify-pack.mjs | 76 +++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 22 deletions(-) diff --git a/scripts/verify-pack.mjs b/scripts/verify-pack.mjs index c6f642c2..279b7e3c 100755 --- a/scripts/verify-pack.mjs +++ b/scripts/verify-pack.mjs @@ -12,9 +12,10 @@ // Each loadable target is loaded in its own child process. One process per // package would let an entry point that calls `process.exit()` — a CLI whose // `.` export runs itself — report success for every target queued behind it. -// The child writes MARKER once the module has evaluated, and only that marker -// counts as a pass; the exit code alone cannot tell "loaded, then exited" from -// "threw while loading". +// A pass needs two independent facts, because either alone is forgeable: the +// child writes MARKER once the module has evaluated, which the exit code cannot +// tell from "threw while loading", and the child must also exit 0, which the +// marker cannot tell from a module that evaluated and then failed. // // Each extracted package gets a `node_modules` holding its declared // dependencies, so a sibling resolves to *its* extracted tarball rather than to @@ -33,7 +34,7 @@ import path from 'node:path'; import process from 'node:process'; import {pathToFileURL} from 'node:url'; -/** @typedef {{subpath: string, conditions: string[], target: string}} Target */ +/** @typedef {{subpath: string, conditions: string[], target: string, unsupported?: string}} Target */ /** @typedef {{name: string, dir: string, source: string, manifest: Record}} Package */ const PACKAGES_DIR = 'packages'; @@ -125,13 +126,26 @@ function collectTargets(subpath, value, conditions = []) { if (typeof value === 'string') { return [{conditions, subpath, target: value}]; } - if (value && typeof value === 'object' && !Array.isArray(value)) { + if (Array.isArray(value)) { + // The fallback-array form: node tries each entry and takes the first that + // resolves, so checking it means resolving the whole list in order. No + // package here uses one. Reported rather than skipped, so that adding one + // is a loud failure instead of a target that is silently never checked. + return [ + { + conditions, + subpath, + target: JSON.stringify(value), + unsupported: 'fallback arrays are not supported', + }, + ]; + } + if (value && typeof value === 'object') { return Object.entries(value).flatMap(([condition, nested]) => collectTargets(subpath, nested, [...conditions, condition]) ); } - // `null` blocks a subpath, and the array fallback form is not used here; - // neither has a file to check. + // `null` blocks a subpath: there is no file to check. return []; } @@ -198,8 +212,8 @@ function targetsOf(manifest) { * @param {Record} manifest * @returns {'import' | 'require' | 'exists' | 'unsupported'} */ -function checkFor({conditions, target}, manifest) { - if (target.includes('*')) { +function checkFor({conditions, target, unsupported}, manifest) { + if (unsupported || target.includes('*')) { return 'unsupported'; } if (conditions.includes('types') || conditions.includes('bin')) { @@ -230,9 +244,10 @@ function describe({conditions, subpath, target}) { } /** - * Loads one target and writes {@link MARKER} if it evaluated. A module that - * calls `process.exit()` while evaluating still executed, so the marker is - * written from an exit handler; only a module that throws suppresses it. + * Loads one target and writes {@link MARKER} if it evaluated cleanly. The + * marker is written from an exit handler so that a module which exits during + * evaluation is still observed, and the handler checks the exit code so that + * one which exits *because it failed* is not counted as a pass. * * @param {'import' | 'require'} mode * @param {string} file @@ -242,8 +257,12 @@ async function loadTarget(mode, file) { let started = false; let threw = false; - process.on('exit', () => { - if (started && !threw) { + // Gated on the exit code as well as on having started: a module that calls + // `process.exit(1)` while evaluating, or that leaves a top-level `await` + // unsettled (node exits 13), reaches this handler having started without + // throwing. Writing the marker for those reports a broken target as a pass. + process.on('exit', (code) => { + if (started && !threw && code === 0) { fs.writeSync(1, MARKER); } }); @@ -306,8 +325,11 @@ async function resolvePackageDir(fromDir, stopDir, dependency) { */ async function packAndExtract(pkg, stageRoot) { // Staging happens outside the workspace so that `npm pack` sees a plain - // package rather than a workspace member. - const stage = path.join(stageRoot, pkg.name.replace(/[@/]/g, '_')); + // package rather than a workspace member. The scope stays a directory rather + // than being flattened into the name: flattening maps both `@a/b_c` and + // `@a_b/c` onto one directory, and two packages staged on top of each other + // pack each other's files. + const stage = path.join(stageRoot, pkg.name); await fsp.cp(pkg.source, stage, { filter: (src) => path.basename(src) !== 'node_modules', recursive: true, @@ -417,15 +439,23 @@ async function findPackages(packagesRoot, extractRoot) { for (const entry of entries.filter((e) => e.isDirectory())) { const source = path.join(scopeRoot, entry.name); - let manifest; + const manifestPath = path.join(source, 'package.json'); + let raw; try { - manifest = JSON.parse( - await fsp.readFile(path.join(source, 'package.json'), 'utf8') - ); + raw = await fsp.readFile(manifestPath, 'utf8'); } catch { - // Not a package — a stray build or cache directory. + // No manifest — a stray build or cache directory, not a package. continue; } + // A manifest that exists but does not parse is a broken package, not a + // non-package. Skipping it here would drop it from the run entirely and + // let the job report that every package it did look at was fine. + let manifest; + try { + manifest = JSON.parse(raw); + } catch (err) { + throw new Error(`${manifestPath} is not valid JSON: ${err.message}`); + } if (!manifest.private) { packages.set(manifest.name, { dir: path.join(extractRoot, manifest.name), @@ -521,7 +551,9 @@ async function checkPackage(pkg) { const check = checkFor(target, pkg.manifest); if (check === 'unsupported') { - problems.push(`${label}: subpath patterns are not supported`); + problems.push( + `${label}: ${target.unsupported ?? 'subpath patterns are not supported'}` + ); continue; } From 96ccda8b6c888d09774a7c8d88268dccfce0abaf Mon Sep 17 00:00:00 2001 From: ianwremmel-ai-agent Date: Mon, 7 Sep 2026 22:50:21 +0000 Subject: [PATCH 5/7] ci(verify-pack): close four ways a broken target could pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the checker found four gaps, each a case where a package that is actually broken is reported as loading cleanly. - A target that resolves outside the extracted directory is now a failure. `path.resolve(pkg.dir, target)` happily accepts an absolute path or one that climbs out with `..`, which lands back in the workspace source — the one place this job exists not to read. - JSON under an explicit `require` condition is now required rather than existence-checked. A file that is present but does not parse is the exact shape the job is for. JSON under any other condition stays an existence check: `import()` of JSON needs a type attribute, so loading every package's `"./package.json"` target would fail for the missing attribute alone. - Two directories claiming one package name is now an error. The map is keyed by name, so the second silently replaced the first and the run reported every package it did look at as passing. - The comments claimed the marker proves the module "has evaluated". It proves evaluation was entered and did not throw; a module that exits partway through still writes it. That is the deliberate price of writing it from an `exit` handler, so the comments now say so. --- scripts/verify-pack.mjs | 112 ++++++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 28 deletions(-) diff --git a/scripts/verify-pack.mjs b/scripts/verify-pack.mjs index 279b7e3c..cdee8e11 100755 --- a/scripts/verify-pack.mjs +++ b/scripts/verify-pack.mjs @@ -12,10 +12,13 @@ // Each loadable target is loaded in its own child process. One process per // package would let an entry point that calls `process.exit()` — a CLI whose // `.` export runs itself — report success for every target queued behind it. -// A pass needs two independent facts, because either alone is forgeable: the -// child writes MARKER once the module has evaluated, which the exit code cannot -// tell from "threw while loading", and the child must also exit 0, which the -// marker cannot tell from a module that evaluated and then failed. +// A pass needs two facts, because either alone is forgeable: the child writes +// MARKER unless loading threw, which an exit code cannot tell from "threw while +// loading", and the child must also exit 0, which the marker cannot tell from a +// module that loaded and then reported a failure. The marker is written from an +// `exit` handler so that a module which exits while evaluating is still +// observed; the price of that is that it proves evaluation was entered and did +// not throw, not that it ran to completion. // // Each extracted package gets a `node_modules` holding its declared // dependencies, so a sibling resolves to *its* extracted tarball rather than to @@ -145,8 +148,21 @@ function collectTargets(subpath, value, conditions = []) { collectTargets(subpath, nested, [...conditions, condition]) ); } - // `null` blocks a subpath: there is no file to check. - return []; + if (value === null) { + // `null` blocks a subpath: there is no file to check. + return []; + } + // A number, a boolean, `undefined` — node rejects all of them as export + // targets. Reported for the same reason as the array form: skipping leaves a + // target that is never checked and a package that passes without being read. + return [ + { + conditions, + subpath, + target: String(value), + unsupported: 'not a string, an array, or an object of conditions', + }, + ]; } /** @@ -179,16 +195,24 @@ function targetsOf(manifest) { throw new Error('package has no "exports" map'); } + // Only a plain object is a map of subpaths and conditions. Anything else — + // a string, an array, or junk — is itself the target for ".". Routing an + // array through Object.entries would turn its indices into condition names + // and drop any non-string entry without checking it. + const isSubpathMap = + typeof exportsMap === 'object' && + exportsMap !== null && + !Array.isArray(exportsMap); + /** @type {Target[]} */ - const targets = - typeof exportsMap === 'string' - ? collectTargets('.', exportsMap) - : Object.entries(exportsMap).flatMap(([key, value]) => - // A key that does not start with "." is a condition on ".". - key.startsWith('.') - ? collectTargets(key, value) - : collectTargets('.', value, [key]) - ); + const targets = isSubpathMap + ? Object.entries(exportsMap).flatMap(([key, value]) => + // A key that does not start with "." is a condition on ".". + key.startsWith('.') + ? collectTargets(key, value) + : collectTargets('.', value, [key]) + ) + : collectTargets('.', exportsMap); if (typeof manifest.types === 'string') { targets.push({ @@ -219,8 +243,14 @@ function checkFor({conditions, target, unsupported}, manifest) { if (conditions.includes('types') || conditions.includes('bin')) { return 'exists'; } + // JSON under an explicit `require` condition is loadable, so load it: a file + // that is present but does not parse is exactly the shape this job exists to + // catch. Any other JSON target is existence-checked, because `import()` of + // JSON needs a type attribute, and an unconditional target — every package's + // `"./package.json": "./package.json"` — would otherwise be `import()`ed by a + // `"type": "module"` package and fail for the missing attribute alone. if (target.endsWith('.json')) { - return 'exists'; + return conditions.includes('require') ? 'require' : 'exists'; } if (conditions.includes('require')) { return 'require'; @@ -244,10 +274,11 @@ function describe({conditions, subpath, target}) { } /** - * Loads one target and writes {@link MARKER} if it evaluated cleanly. The - * marker is written from an exit handler so that a module which exits during - * evaluation is still observed, and the handler checks the exit code so that - * one which exits *because it failed* is not counted as a pass. + * Loads one target, writing {@link MARKER} unless loading threw. The marker is + * written from an `exit` handler so that a module which exits while evaluating + * is still observed — which is also why it proves only that evaluation was + * entered and did not throw. Whether the load *succeeded* is the exit code's + * job, and {@link checkPackage} reads that. * * @param {'import' | 'require'} mode * @param {string} file @@ -257,12 +288,12 @@ async function loadTarget(mode, file) { let started = false; let threw = false; - // Gated on the exit code as well as on having started: a module that calls - // `process.exit(1)` while evaluating, or that leaves a top-level `await` - // unsettled (node exits 13), reaches this handler having started without - // throwing. Writing the marker for those reports a broken target as a pass. - process.on('exit', (code) => { - if (started && !threw && code === 0) { + // The marker says only that evaluation was reached and did not throw. Whether + // it *succeeded* is the exit code's job, and the caller reads that, because + // the code passed to this handler is not final — a handler registered later + // can still change `process.exitCode`. + process.on('exit', () => { + if (started && !threw) { fs.writeSync(1, MARKER); } }); @@ -282,7 +313,10 @@ async function loadTarget(mode, file) { // The module loaded. Leaving normally would wait on whatever handles it // opened, so stop here rather than hanging on a timer or an open socket. - process.exit(0); + // Exiting with whatever status the module asked for rather than a flat 0: a + // module that set `process.exitCode` while evaluating is reporting a failure, + // and overriding it here would hide that from the caller. + process.exit(process.exitCode ?? 0); } /** @@ -457,6 +491,15 @@ async function findPackages(packagesRoot, extractRoot) { throw new Error(`${manifestPath} is not valid JSON: ${err.message}`); } if (!manifest.private) { + // Keying by name means a second directory claiming the same name would + // overwrite the first, dropping it from the run while the final count + // still reports every package it did look at as passing. + const existing = packages.get(manifest.name); + if (existing) { + throw new Error( + `${manifest.name} is declared by both ${existing.source} and ${source}` + ); + } packages.set(manifest.name, { dir: path.join(extractRoot, manifest.name), manifest, @@ -558,6 +601,14 @@ async function checkPackage(pkg) { } const file = path.resolve(pkg.dir, target.target); + // Reading the tarball is the whole point, so a target that resolves out of + // the extracted directory — an absolute path, or one that climbs out with + // `..` — is a failure rather than something to load from wherever it landed. + // Without this it could resolve back into the workspace source and pass. + if (file !== pkg.dir && !file.startsWith(`${pkg.dir}${path.sep}`)) { + problems.push(`${label}: resolves outside the extracted package`); + continue; + } if (!fs.existsSync(file)) { problems.push(`${label}: missing from the tarball`); continue; @@ -576,7 +627,12 @@ async function checkPackage(pkg) { problems.push( `${label}: ${check}() did not finish within ${CHILD_TIMEOUT_MS / 1000}s` ); - } else if (!result.stdout.includes(MARKER)) { + } else if (!result.stdout.includes(MARKER) || result.code !== 0) { + // Both are required. Without the marker, a module that threw while + // evaluating is indistinguishable from one that loaded and exited; + // without a zero exit code, a module that evaluated and then reported a + // failure — `process.exit(1)` mid-evaluation, an unsettled top-level + // `await` (node exits 13), a non-zero `process.exitCode` — passes. const detail = `${result.stderr}${result.stdout.replaceAll(MARKER, '')}`.trim(); problems.push( From 29b3e9ec50fd6b128439884035a4e0fe884baa32 Mon Sep 17 00:00:00 2001 From: ianwremmel-ai-agent Date: Tue, 8 Sep 2026 00:05:10 +0000 Subject: [PATCH 6/7] ci(verify-pack): split checkFor and checkPackage to satisfy the complexity rule ESLint's `complexity` rule caps a function at 10; `checkFor` reached 12 and `checkPackage` 13 in 96ccda8, which is what failed the lint job. Both are decomposed rather than exempted. `checkFor` hands its extension and `type` fallback to a new `loaderFor`; `checkPackage` hands one target to `checkTarget` and the outcome of one child load to `loadProblem`, leaving it to collect what those return. No behavior change. A differential test over the 968 reachable combinations of conditions, target extension, `unsupported`, package `type`, exit code, stdout shape and timeout flag produces identical results before and after. --- scripts/verify-pack.mjs | 139 ++++++++++++++++++++++++---------------- 1 file changed, 84 insertions(+), 55 deletions(-) diff --git a/scripts/verify-pack.mjs b/scripts/verify-pack.mjs index cdee8e11..0bb31c04 100755 --- a/scripts/verify-pack.mjs +++ b/scripts/verify-pack.mjs @@ -225,6 +225,24 @@ function targetsOf(manifest) { return [...targets, ...binTargets(manifest.bin)]; } +/** + * The loader for a target that names neither `import` nor `require`: the + * extension decides where it is explicit, and the package's `type` otherwise. + * + * @param {string} target + * @param {Record} manifest + * @returns {'import' | 'require'} + */ +function loaderFor(target, manifest) { + if (target.endsWith('.cjs')) { + return 'require'; + } + if (target.endsWith('.mjs')) { + return 'import'; + } + return manifest.type === 'module' ? 'import' : 'require'; +} + /** * How to check one target. `types` targets are declarations, `bin` targets are * programs and JSON is data, so those get an existence check. Everything else @@ -258,13 +276,7 @@ function checkFor({conditions, target, unsupported}, manifest) { if (conditions.includes('import')) { return 'import'; } - if (target.endsWith('.cjs')) { - return 'require'; - } - if (target.endsWith('.mjs')) { - return 'import'; - } - return manifest.type === 'module' ? 'import' : 'require'; + return loaderFor(target, manifest); } /** @param {Target} target */ @@ -573,6 +585,68 @@ async function prepare(packages, stageRoot, workspaceRoot) { return failures; } +/** + * Reads the outcome of one child load. + * + * @param {string} label + * @param {'import' | 'require'} check + * @param {{code: number, stdout: string, stderr: string, timedOut: boolean}} result + * @returns {string | null} the failure, or null if the target loaded + */ +function loadProblem(label, check, result) { + if (result.timedOut) { + return `${label}: ${check}() did not finish within ${CHILD_TIMEOUT_MS / 1000}s`; + } + // Both are required. Without the marker, a module that threw while + // evaluating is indistinguishable from one that loaded and exited; without a + // zero exit code, a module that evaluated and then reported a failure — + // `process.exit(1)` mid-evaluation, an unsettled top-level `await` (node + // exits 13), a non-zero `process.exitCode` — passes. + if (result.stdout.includes(MARKER) && result.code === 0) { + return null; + } + const detail = + `${result.stderr}${result.stdout.replaceAll(MARKER, '')}`.trim(); + return `${label}: ${check}() failed${detail ? `: ${detail}` : ` with exit code ${result.code}`}`; +} + +/** + * @param {Package} pkg + * @param {Target} target + * @returns {Promise} the failure, or null if the target is fine + */ +async function checkTarget(pkg, target) { + const label = describe(target); + const check = checkFor(target, pkg.manifest); + + if (check === 'unsupported') { + return `${label}: ${target.unsupported ?? 'subpath patterns are not supported'}`; + } + + const file = path.resolve(pkg.dir, target.target); + // Reading the tarball is the whole point, so a target that resolves out of + // the extracted directory — an absolute path, or one that climbs out with + // `..` — is a failure rather than something to load from wherever it landed. + // Without this it could resolve back into the workspace source and pass. + if (file !== pkg.dir && !file.startsWith(`${pkg.dir}${path.sep}`)) { + return `${label}: resolves outside the extracted package`; + } + if (!fs.existsSync(file)) { + return `${label}: missing from the tarball`; + } + if (check === 'exists') { + return null; + } + + const result = await run( + process.execPath, + [import.meta.filename, '--load', check, file], + pkg.dir + ); + + return loadProblem(label, check, result); +} + /** * @param {Package} pkg * @returns {Promise} one line per failed target @@ -590,54 +664,9 @@ async function checkPackage(pkg) { } for (const target of targets) { - const label = describe(target); - const check = checkFor(target, pkg.manifest); - - if (check === 'unsupported') { - problems.push( - `${label}: ${target.unsupported ?? 'subpath patterns are not supported'}` - ); - continue; - } - - const file = path.resolve(pkg.dir, target.target); - // Reading the tarball is the whole point, so a target that resolves out of - // the extracted directory — an absolute path, or one that climbs out with - // `..` — is a failure rather than something to load from wherever it landed. - // Without this it could resolve back into the workspace source and pass. - if (file !== pkg.dir && !file.startsWith(`${pkg.dir}${path.sep}`)) { - problems.push(`${label}: resolves outside the extracted package`); - continue; - } - if (!fs.existsSync(file)) { - problems.push(`${label}: missing from the tarball`); - continue; - } - if (check === 'exists') { - continue; - } - - const result = await run( - process.execPath, - [import.meta.filename, '--load', check, file], - pkg.dir - ); - - if (result.timedOut) { - problems.push( - `${label}: ${check}() did not finish within ${CHILD_TIMEOUT_MS / 1000}s` - ); - } else if (!result.stdout.includes(MARKER) || result.code !== 0) { - // Both are required. Without the marker, a module that threw while - // evaluating is indistinguishable from one that loaded and exited; - // without a zero exit code, a module that evaluated and then reported a - // failure — `process.exit(1)` mid-evaluation, an unsettled top-level - // `await` (node exits 13), a non-zero `process.exitCode` — passes. - const detail = - `${result.stderr}${result.stdout.replaceAll(MARKER, '')}`.trim(); - problems.push( - `${label}: ${check}() failed${detail ? `: ${detail}` : ` with exit code ${result.code}`}` - ); + const problem = await checkTarget(pkg, target); + if (problem) { + problems.push(problem); } } From 8e2d0384eb2945c3232722bee176de38fed7a9ca Mon Sep 17 00:00:00 2001 From: ianwremmel-ai-agent Date: Tue, 8 Sep 2026 00:05:44 +0000 Subject: [PATCH 7/7] ci(verify-pack): fail the async and empty-discovery silent passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more adversarial review rounds on Codex, one spec-aware and one spec-blind. Four findings were real, each a way a broken target or a broken checkout is reported as clean. - A module that fails on a later turn now fails. The child exited the instant the loader returned, so a top-level `Promise.reject()` or a `setImmediate` that throws never reached the event loop and the target passed. The child now installs `unhandledRejection` and `uncaughtException` handlers and defers its exit by one `setImmediate` — one turn, enough to see what is already queued, and still bounded, so a module holding a socket open cannot hang the job. - Finding no packages is now an error. An empty discovery printed "All 0 packages load from their tarball" and exited 0, so a moved packages directory or a broken checkout would have turned the job green. - Only ENOENT means "not a package" when reading a manifest. Every other error — a permission problem, an I/O error — was swallowed as a stray directory, dropping a real package from the run while the job still passed. - A non-string `bin` entry is reported rather than dropped, matching what `collectTargets` already does with the export shapes it cannot check. Verified against a synthetic workspace: packages whose entry point calls `Promise.reject()` or `setImmediate(() => { throw })` passed before and fail now; one that calls `process.exit(0)` mid-evaluation still passes, which is the documented tradeoff of writing the marker from an `exit` handler. The empty workspace exited 0 before and exits 1 now. Against the real 32 packages the result is unchanged — the same 30 pass and the same two fail on the `bin` that #435 removes — so nothing here is a false positive. Dismissed, with the evidence that settles each: no package declares `peerDependencies`, `bundledDependencies`, or a `prepack`/`prepare` script; every `bin` and `types` value is a string; the only JSON target is the unconditional `"./package.json"`, never under an `import` condition; and the only conditions in use are `types`, `default`, `require` and `import`, so custom-condition resolution does not arise. Static `exports`-map validity — a target that does not start with `./`, subpath and condition keys mixed — stays publint's job under CLC-1134. A module that calls `process.exit(0)` while evaluating still passes by design. --- scripts/verify-pack.mjs | 77 ++++++++++++++++++++++++++++++----------- 1 file changed, 57 insertions(+), 20 deletions(-) diff --git a/scripts/verify-pack.mjs b/scripts/verify-pack.mjs index 0bb31c04..a5073164 100755 --- a/scripts/verify-pack.mjs +++ b/scripts/verify-pack.mjs @@ -174,13 +174,20 @@ function binTargets(bin) { return [{conditions: ['bin'], subpath: 'bin', target: bin}]; } if (bin && typeof bin === 'object') { - return Object.entries(bin) - .filter(([, target]) => typeof target === 'string') - .map(([name, target]) => ({ - conditions: ['bin'], - subpath: `bin.${name}`, - target, - })); + // A non-string entry is reported rather than dropped, for the same reason + // `collectTargets` reports the shapes it cannot check: skipping it leaves a + // declared program that is never looked at and a package that passes + // anyway. + return Object.entries(bin).map(([name, target]) => + typeof target === 'string' + ? {conditions: ['bin'], subpath: `bin.${name}`, target} + : { + conditions: ['bin'], + subpath: `bin.${name}`, + target: String(target), + unsupported: 'bin target is not a string', + } + ); } return []; } @@ -286,11 +293,11 @@ function describe({conditions, subpath, target}) { } /** - * Loads one target, writing {@link MARKER} unless loading threw. The marker is + * Loads one target, writing {@link MARKER} unless loading failed. The marker is * written from an `exit` handler so that a module which exits while evaluating * is still observed — which is also why it proves only that evaluation was - * entered and did not throw. Whether the load *succeeded* is the exit code's - * job, and {@link checkPackage} reads that. + * entered and nothing was thrown. Whether the load *succeeded* is the exit + * code's job, and {@link loadProblem} reads that. * * @param {'import' | 'require'} mode * @param {string} file @@ -300,16 +307,23 @@ async function loadTarget(mode, file) { let started = false; let threw = false; - // The marker says only that evaluation was reached and did not throw. Whether - // it *succeeded* is the exit code's job, and the caller reads that, because - // the code passed to this handler is not final — a handler registered later - // can still change `process.exitCode`. + // The marker says only that evaluation was reached and nothing was thrown. + // Whether it *succeeded* is the exit code's job, and the caller reads that, + // because the code passed to this handler is not final — a handler registered + // later can still change `process.exitCode`. process.on('exit', () => { if (started && !threw) { fs.writeSync(1, MARKER); } }); + /** @param {unknown} err */ + const fail = (err) => { + threw = true; + process.stderr.write(`${err instanceof Error ? err.stack : err}\n`); + process.exit(1); + }; + try { started = true; if (mode === 'require') { @@ -318,17 +332,27 @@ async function loadTarget(mode, file) { await import(pathToFileURL(file).href); } } catch (err) { - threw = true; - process.stderr.write(`${err instanceof Error ? err.stack : err}\n`); - process.exit(1); + fail(err); } + // A module whose initialisation fails on a later turn — a top-level + // `Promise.reject()`, a `setImmediate` that throws — is as broken as one that + // throws while evaluating, and exiting the instant the loader returns would + // report it as clean. These catch it. + process.on('unhandledRejection', fail); + process.on('uncaughtException', fail); + // The module loaded. Leaving normally would wait on whatever handles it // opened, so stop here rather than hanging on a timer or an open socket. + // `setImmediate` rather than exiting outright gives the event loop exactly one + // turn, which is enough for the handlers above to see a rejection or a throw + // already queued, and is still bounded — anything the module scheduled for + // later is not this job's business. + // // Exiting with whatever status the module asked for rather than a flat 0: a // module that set `process.exitCode` while evaluating is reporting a failure, // and overriding it here would hide that from the caller. - process.exit(process.exitCode ?? 0); + setImmediate(() => process.exit(process.exitCode ?? 0)); } /** @@ -489,8 +513,14 @@ async function findPackages(packagesRoot, extractRoot) { let raw; try { raw = await fsp.readFile(manifestPath, 'utf8'); - } catch { - // No manifest — a stray build or cache directory, not a package. + } catch (err) { + // Only "there is no manifest here" means "not a package" — a stray build + // or cache directory. Any other error is this job failing to read a + // package it should have checked, and swallowing it would drop that + // package from the run while the job still reported success. + if (err.code !== 'ENOENT') { + throw new Error(`could not read ${manifestPath}: ${err.message}`); + } continue; } // A manifest that exists but does not parse is a broken package, not a @@ -522,6 +552,13 @@ async function findPackages(packagesRoot, extractRoot) { } } + // Finding nothing is a broken checkout or a moved packages directory, not a + // clean run. Without this the job reports "All 0 packages load" and goes + // green, which is the loudest silent pass available to it. + if (packages.size === 0) { + throw new Error(`no publishable packages found under ${packagesRoot}`); + } + return new Map( [...packages.entries()].sort(([a], [b]) => a.localeCompare(b)) );