* test(#2858): add class-extinction guard for shipped-script require boundary Every shipped scripts/**/*.cjs must be require-able using only shipped paths. The guard resolves the tarball file list via npm pack --dry-run --json (not a hardcoded list) and statically checks each require() call against the shipped set. A script requiring ../tests/** (which does not ship) is a violation. * fix(#2858): exclude gen-emitted-baseline.cjs from the npm tarball scripts/gen-emitted-baseline.cjs is repo-only CI tooling (CI workflows + test fixtures spawn it from a checkout). It requires three modules from tests/, which does not ship — so in a published install it is MODULE_NOT_FOUND at load time. Add a targeted files[] negation (!scripts/gen-emitted-baseline.cjs) so the script stays in the repo for CI use but does not ship. Other scripts that ship and are required by bin/install.js (build-hooks.js, fix-slash-commands.cjs, gen-capability-registry.cjs) are unaffected. The class-extinction guard test in tests/packaging-shipped-scripts-require-only-shipped.test.cjs ensures no shipped script can require outside the shipped tree going forward. * test(#2858): widen guard to .js + strip block comments (review fixes) Two findings from isolated adversarial review: 1. MAJOR: the guard only checked .cjs files, but scripts/build-hooks.js ships and is required by bin/install.js — a .js file with a broken require would bypass the guard. Widened filter to /\.(cjs|js)$/. 2. MODERATE: the static parser could false-positive on require() calls inside /* */ block comments or inline // comments. Now strips both before matching. * chore(#2858): add changeset fragment * chore(#2858): backfill changeset PR number 2968 * fix(#2858): add issue ref to allow-test-rule exemption (ADR-456) CI lint-tests caught: the allow-test-rule comment needs a 'see #NNN' ref per ADR-456. Added '(see #2858)' to the integration-test-input exemption. --------- Co-authored-by: sim <sim@local>
This commit is contained in:
5
.changeset/sturdy-sloths-forage.md
Normal file
5
.changeset/sturdy-sloths-forage.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 2968
|
||||
---
|
||||
**Published installs no longer crash on a script that can't load** — `scripts/gen-emitted-baseline.cjs` shipped in the npm tarball but required three modules from `tests/` (which does not ship), producing `MODULE_NOT_FOUND` at load time. The script is repo-only CI tooling and is now excluded from the tarball. A class-extinction guard test ensures no shipped script can require outside the shipped tree going forward. (#2858)
|
||||
@@ -21,6 +21,7 @@
|
||||
"GEMINI.md",
|
||||
"hooks",
|
||||
"scripts",
|
||||
"!scripts/gen-emitted-baseline.cjs",
|
||||
"pi",
|
||||
"vscode"
|
||||
],
|
||||
|
||||
180
tests/packaging-shipped-scripts-require-only-shipped.test.cjs
Normal file
180
tests/packaging-shipped-scripts-require-only-shipped.test.cjs
Normal file
@@ -0,0 +1,180 @@
|
||||
// allow-test-rule: integration-test-input (see #2858)
|
||||
// #2858 — class-extinction guard: every shipped scripts/**/*.cjs must be
|
||||
// require-able using only shipped paths. A script that ships in the npm tarball
|
||||
// but requires a module from tests/ (which does not ship) is MODULE_NOT_FOUND
|
||||
// at load time in a published install.
|
||||
//
|
||||
// This test statically checks each shipped .cjs's require() calls against the
|
||||
// resolved tarball file list (npm pack --dry-run --json), so adding a new entry
|
||||
// to `files` cannot silently opt out. It does NOT execute the scripts — it parses
|
||||
// their require() calls and resolves them against the shipped set.
|
||||
|
||||
'use strict';
|
||||
|
||||
process.env.GSD_TEST_MODE = '1';
|
||||
|
||||
const { describe, test, before } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { execFileSync } = require('node:child_process');
|
||||
|
||||
const REPO_ROOT = path.join(__dirname, '..');
|
||||
|
||||
/**
|
||||
* Resolve the tarball file list via `npm pack --dry-run --json`.
|
||||
* This is the ACTUAL set of files that ship — not a hardcoded list — so adding
|
||||
* a new entry to package.json `files` cannot silently bypass the guard.
|
||||
*/
|
||||
function resolveTarballFiles() {
|
||||
const raw = execFileSync('npm', ['pack', '--dry-run', '--json'], {
|
||||
cwd: REPO_ROOT,
|
||||
encoding: 'utf-8',
|
||||
shell: true, // Windows: npm is npm.cmd and needs a shell
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
timeout: 60_000,
|
||||
});
|
||||
const parsed = JSON.parse(raw);
|
||||
return new Set(parsed[0].files.map((f) => f.path.replace(/\\/g, '/')));
|
||||
}
|
||||
|
||||
/**
|
||||
* Extract all require('...') string-literal calls from a .cjs source.
|
||||
* Only static string-literal requires are checked — dynamic require(variable)
|
||||
* is out of scope (and would itself be a red flag in a shipped script).
|
||||
*/
|
||||
function extractRequires(source) {
|
||||
const requires = [];
|
||||
// Strip block comments (/* ... */) and inline line comments (// ...) before
|
||||
// matching, so a require() appearing in a doc comment or fenced code block
|
||||
// inside a /* */ does not produce a false positive.
|
||||
const stripped = source
|
||||
.replace(/\/\*[\s\S]*?\*\//g, '')
|
||||
.replace(/\/\/.*$/gm, '');
|
||||
const requireRe = /\brequire\s*\(\s*['"]([^'"]+)['"]\s*\)/g;
|
||||
let m;
|
||||
while ((m = requireRe.exec(stripped)) !== null) {
|
||||
requires.push(m[1]);
|
||||
}
|
||||
return requires;
|
||||
}
|
||||
|
||||
/**
|
||||
* Classify a require specifier from a given file path:
|
||||
* - 'builtin' — node: prefix or a Node builtin (fs, path, etc.)
|
||||
* - 'shipped' — resolves to a file within the shipped tarball set
|
||||
* - 'external' — an npm dependency (resolvable from node_modules)
|
||||
* - 'unshipped' — resolves to a repo path that is NOT in the shipped set (VIOLATION)
|
||||
*/
|
||||
function classifyRequire(spec, fromFile, shippedFiles) {
|
||||
// Node builtins (including node: prefix)
|
||||
if (spec.startsWith('node:')) return 'builtin';
|
||||
const BUILTINS = ['fs', 'path', 'os', 'child_process', 'crypto', 'util', 'url', 'events', 'stream', 'http', 'https', 'net', 'tls', 'zlib', 'querystring', 'string_decoder', 'timers', 'vm', 'worker_threads', 'buffer', 'process'];
|
||||
const bare = spec.split('/')[0];
|
||||
if (BUILTINS.includes(bare)) return 'builtin';
|
||||
|
||||
// Relative/absolute requires — resolve against the file's directory
|
||||
if (spec.startsWith('.') || spec.startsWith('/')) {
|
||||
const fromDir = path.dirname(fromFile);
|
||||
const resolved = path.posix.normalize(path.posix.join(fromDir, spec));
|
||||
// Try exact, .cjs, .js, .json, /index.cjs, /index.js
|
||||
const candidates = [
|
||||
resolved,
|
||||
resolved + '.cjs',
|
||||
resolved + '.js',
|
||||
resolved + '.json',
|
||||
resolved + '/index.cjs',
|
||||
resolved + '/index.js',
|
||||
];
|
||||
for (const c of candidates) {
|
||||
const normalized = c.replace(/\\/g, '/');
|
||||
if (shippedFiles.has(normalized)) return 'shipped';
|
||||
}
|
||||
// If it resolves to a real file on disk but is NOT shipped → violation
|
||||
const absResolved = path.resolve(REPO_ROOT, resolved);
|
||||
const absCandidates = [
|
||||
absResolved,
|
||||
absResolved + '.cjs',
|
||||
absResolved + '.js',
|
||||
absResolved + '.json',
|
||||
path.join(absResolved, 'index.cjs'),
|
||||
path.join(absResolved, 'index.js'),
|
||||
];
|
||||
for (const c of absCandidates) {
|
||||
if (fs.existsSync(c)) return 'unshipped';
|
||||
}
|
||||
// Doesn't resolve to anything — likely a typo or generated artifact; flag it
|
||||
return 'unshipped';
|
||||
}
|
||||
|
||||
// Bare specifier (not relative, not builtin) → npm dependency
|
||||
return 'external';
|
||||
}
|
||||
|
||||
describe('#2858 — shipped scripts require only shipped paths', () => {
|
||||
let shippedFiles;
|
||||
let shippedScripts;
|
||||
|
||||
before(() => {
|
||||
shippedFiles = resolveTarballFiles();
|
||||
// Filter to scripts/**/*.{cjs,js} (the shipped script surface — both .cjs
|
||||
// and .js files ship under scripts/, e.g. build-hooks.js is required by
|
||||
// bin/install.js). Both extensions are checked so a broken require() in a
|
||||
// .js file is caught just the same as one in a .cjs file.
|
||||
shippedScripts = [...shippedFiles]
|
||||
.filter((f) => f.startsWith('scripts/') && /\.(cjs|js)$/.test(f))
|
||||
.sort();
|
||||
});
|
||||
|
||||
test('every shipped scripts/*.{cjs,js} is require-able from a shipped-only tree', () => {
|
||||
assert.ok(shippedScripts.length > 0, 'expected at least one shipped script');
|
||||
|
||||
const violations = [];
|
||||
for (const scriptRel of shippedScripts) {
|
||||
const absPath = path.join(REPO_ROOT, scriptRel);
|
||||
if (!fs.existsSync(absPath)) continue; // generated artifact, skip
|
||||
const source = fs.readFileSync(absPath, 'utf-8');
|
||||
const requires = extractRequires(source);
|
||||
|
||||
for (const spec of requires) {
|
||||
const classification = classifyRequire(spec, scriptRel, shippedFiles);
|
||||
if (classification === 'unshipped') {
|
||||
violations.push({
|
||||
script: scriptRel,
|
||||
require: spec,
|
||||
classification,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (violations.length > 0) {
|
||||
const details = violations
|
||||
.map((v) => ` ${v.script}: require('${v.require}') → ${v.classification}`)
|
||||
.join('\n');
|
||||
assert.fail(
|
||||
`${violations.length} shipped script(s) require modules outside the shipped tree (MODULE_NOT_FOUND in a published install):\n${details}`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('gen-emitted-baseline.cjs does NOT ship (repo-only CI tooling)', () => {
|
||||
// This script requires ../tests/helpers/* which does not ship. It is
|
||||
// repo-only CI tooling (CI workflows + test fixtures spawn it from a
|
||||
// checkout). It must be excluded from the npm tarball.
|
||||
assert.ok(
|
||||
!shippedFiles.has('scripts/gen-emitted-baseline.cjs'),
|
||||
'scripts/gen-emitted-baseline.cjs must NOT ship — it requires tests/ which does not ship (#2858)',
|
||||
);
|
||||
});
|
||||
|
||||
test('a script requiring a sibling in the same shipped dir resolves as shipped (positive case)', () => {
|
||||
// Sanity: scripts/lib/cli-exit.cjs ships and is required by shipped scripts.
|
||||
// This confirms the guard's positive path works — a valid intra-shipped require
|
||||
// is NOT flagged.
|
||||
assert.ok(shippedFiles.has('scripts/lib/cli-exit.cjs'), 'scripts/lib/cli-exit.cjs should ship');
|
||||
const classification = classifyRequire('./lib/cli-exit.cjs', 'scripts/gen-adr-index.cjs', shippedFiles);
|
||||
assert.strictEqual(classification, 'shipped',
|
||||
`a sibling require within scripts/ must classify as 'shipped'; got '${classification}'`);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user