The first CI run failed on `node:test`, in every test file, reported as the module boundary being broken. It was not: `builtinModules` omits `test` on Node 20 (CI) and includes it on Node 24 (local), so a list rebuilt from it disagrees with itself across versions. Use `isBuiltin`, which is Node's own answer, and treat the `node:` prefix as sufficient on its own -- a prefixed specifier can never resolve to a package, whatever the running version enumerates. Test covers both forms. Also corrects this file's header: the client half's guard is no longer `external` (it never worked), it is the Vite build's resolution-time check plus checkExternals.js on the built chunk. Co-Authored-By: Claude <noreply@anthropic.com>
184 lines
8.3 KiB
JavaScript
184 lines
8.3 KiB
JavaScript
#!/usr/bin/env node
|
|
// ── §5.1 — zero internal imports ───────────────────────────────────────────
|
|
//
|
|
// The acceptance test for the whole module contract. A module that reaches into
|
|
// core's tree still works — right up until core moves a file — and the boundary
|
|
// this workstream exists to build is worth exactly as much as this check is.
|
|
//
|
|
// MODULE_API.md §5.1 sketches it as a grep for `../../`. That is the shape of
|
|
// the violation but not the rule, and the difference matters in both directions:
|
|
// a grep says nothing about `require('../../../../etc/passwd')` from a deeply
|
|
// nested file (which it catches by accident) and false-alarms on a legitimate
|
|
// `require('../module.json')` from `server/` (which it catches wrongly). So this
|
|
// RESOLVES each specifier against the file that wrote it and asks whether the
|
|
// result is still inside the module root — the actual rule, stated once.
|
|
//
|
|
// Bare specifiers are checked too, and against a stricter list than "is it
|
|
// installed": core hands the module express, express-validator, the database and
|
|
// the logger on `ctx` precisely so the module never resolves them, and Node's
|
|
// resolver cannot reach core's `node_modules` from here anyway. A bare
|
|
// `require` that is not a Node builtin is therefore a module that will fail to
|
|
// load on a real install, with a message about a missing package rather than
|
|
// about the rule it broke.
|
|
//
|
|
// **That second check applies to SHIPPED code only.** `test/` and `scripts/`
|
|
// never run inside core's process — the fakes in `test/_fakes.js` build a real
|
|
// `express` router precisely so the module's routers are exercised for real —
|
|
// so they may use devDependencies. The containment check applies everywhere,
|
|
// because a test that reaches into core's tree is a test that passes on this
|
|
// machine and nowhere else.
|
|
//
|
|
// Run over the SERVER half. The client half's equivalents are its Vite build,
|
|
// which fails if a shared dependency resolves into node_modules, and
|
|
// client/scripts/checkExternals.js, which asks the built chunk whether any bare
|
|
// specifier survived.
|
|
|
|
const fs = require('fs')
|
|
const path = require('path')
|
|
// Node's own answer, not a list reconstructed from `builtinModules`. That list
|
|
// omits `test` on Node 20 and includes it on Node 24, so a suite that requires
|
|
// `node:test` passed locally and failed in CI on the very first run — reported
|
|
// as the module boundary being broken, which it was not. `isBuiltin` is the
|
|
// authoritative check and handles the `node:` prefix itself.
|
|
const { isBuiltin } = require('module')
|
|
|
|
const MODULE_ROOT = path.resolve(__dirname, '..', '..')
|
|
const SERVER_ROOT = path.join(MODULE_ROOT, 'server')
|
|
|
|
// Dependencies this half is allowed to resolve for itself. Empty, and that is
|
|
// the design: everything the server half needs comes from `ctx` (§2.3). A new
|
|
// entry here is a real decision — it becomes a package an operator's install
|
|
// has to carry — so it should be argued for in a PR, not added in passing.
|
|
const ALLOWED_PACKAGES = new Set([])
|
|
|
|
const SKIP_DIRS = new Set(['node_modules', 'coverage', '.git'])
|
|
|
|
// Directories whose contents never run inside core's process, and may therefore
|
|
// resolve this package's devDependencies.
|
|
const NOT_SHIPPED = [path.join(SERVER_ROOT, 'test'), path.join(SERVER_ROOT, 'scripts')]
|
|
const isShipped = (file) => !NOT_SHIPPED.some((d) => file.startsWith(d + path.sep))
|
|
|
|
const devDependencies = new Set(
|
|
Object.keys(JSON.parse(fs.readFileSync(path.join(SERVER_ROOT, 'package.json'), 'utf8')).devDependencies || {}),
|
|
)
|
|
|
|
// `require('x')`, `from 'x'`, `import('x')`. Deliberately textual: parsing would
|
|
// need a dependency, and a specifier this pattern misses is a specifier written
|
|
// to be missed, which review catches and a stricter regexp would not.
|
|
const SPECIFIER = /(?:require\(|from\s+|import\()\s*['"]([^'"]+)['"]/g
|
|
|
|
/**
|
|
* Blank out comments and template literals before scanning.
|
|
*
|
|
* Not a nicety — without it this file fails on ITSELF, because the comments
|
|
* above name `require('../../../../etc/passwd')` as an example of what to
|
|
* catch, and index.js explains in prose why it must never `require('express')`.
|
|
* A boundary check that cannot survive being described is a check people stop
|
|
* writing comments around.
|
|
*
|
|
* A character walk rather than a regexp, because the two get in each other's
|
|
* way: `'https://x'` contains a line-comment opener inside a string, and
|
|
* `// don't` contains a quote inside a comment. Tracking the state is shorter
|
|
* than the regexp that would almost handle it. Content is replaced with spaces
|
|
* rather than removed so nothing else has to care.
|
|
*/
|
|
function stripCommentsAndTemplates(src) {
|
|
let out = ''
|
|
let i = 0
|
|
const keep = (n) => { out += src.slice(i, i + n); i += n }
|
|
const blank = (end) => { out += src.slice(i, end).replace(/[^\n]/g, ' '); i = end }
|
|
while (i < src.length) {
|
|
const two = src.slice(i, i + 2)
|
|
if (two === '//') {
|
|
const nl = src.indexOf('\n', i)
|
|
blank(nl === -1 ? src.length : nl)
|
|
} else if (two === '/*') {
|
|
const end = src.indexOf('*/', i + 2)
|
|
blank(end === -1 ? src.length : end + 2)
|
|
} else if (src[i] === '"' || src[i] === "'") {
|
|
// Strings are KEPT — they are where the specifiers live.
|
|
const quote = src[i]
|
|
keep(1)
|
|
while (i < src.length && src[i] !== quote) keep(src[i] === '\\' ? 2 : 1)
|
|
keep(1)
|
|
} else if (src[i] === '`') {
|
|
// Template literals are blanked: nothing may `require` a template, and a
|
|
// template holding SQL or HTML is a rich source of false positives.
|
|
i += 1
|
|
out += ' '
|
|
while (i < src.length && src[i] !== '`') {
|
|
if (src[i] === '\\') { out += ' '; i += 2 } else { out += src[i] === '\n' ? '\n' : ' '; i += 1 }
|
|
}
|
|
i += 1
|
|
out += ' '
|
|
} else {
|
|
keep(1)
|
|
}
|
|
}
|
|
return out
|
|
}
|
|
|
|
function* walk(dir) {
|
|
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
|
|
if (entry.isDirectory()) {
|
|
if (!SKIP_DIRS.has(entry.name)) yield* walk(path.join(dir, entry.name))
|
|
} else if (/\.(js|mjs|cjs)$/.test(entry.name)) {
|
|
yield path.join(dir, entry.name)
|
|
}
|
|
}
|
|
}
|
|
|
|
/**
|
|
* Every boundary violation under `root`, resolved against `moduleRoot`.
|
|
*
|
|
* Exported so `test/checkImports.test.js` can point it at fixtures. A check that
|
|
* has never been shown to fail is a check nobody knows the state of — and this
|
|
* one guards the acceptance criterion for the whole contract.
|
|
*/
|
|
function scan(root, moduleRoot = MODULE_ROOT, { shipped = isShipped, dev = devDependencies } = {}) {
|
|
const violations = []
|
|
for (const file of walk(root)) {
|
|
const source = stripCommentsAndTemplates(fs.readFileSync(file, 'utf8'))
|
|
for (const [, specifier] of source.matchAll(SPECIFIER)) {
|
|
if (specifier.startsWith('.')) {
|
|
const resolved = path.resolve(path.dirname(file), specifier)
|
|
if (resolved !== moduleRoot && !resolved.startsWith(moduleRoot + path.sep)) {
|
|
violations.push({ file, specifier, why: 'escapes the module root' })
|
|
}
|
|
} else if (path.isAbsolute(specifier)) {
|
|
violations.push({ file, specifier, why: 'absolute path' })
|
|
} else {
|
|
const pkg = specifier.startsWith('@')
|
|
? specifier.split('/').slice(0, 2).join('/')
|
|
: specifier.split('/')[0]
|
|
const allowed = ALLOWED_PACKAGES.has(pkg) || (!shipped(file) && dev.has(pkg))
|
|
// The `node:` prefix can only ever name a builtin, so it never reaches
|
|
// node_modules and is safe whatever this Node version enumerates.
|
|
const builtin = isBuiltin(specifier) || specifier.startsWith('node:')
|
|
if (!builtin && !allowed) {
|
|
violations.push({ file, specifier, why: 'undeclared bare specifier — should this come from ctx?' })
|
|
}
|
|
}
|
|
}
|
|
}
|
|
return violations
|
|
}
|
|
|
|
module.exports = { scan, stripCommentsAndTemplates, SERVER_ROOT, MODULE_ROOT }
|
|
|
|
// Required by a test, or run as the check? Only the second one exits.
|
|
if (require.main !== module) return
|
|
|
|
const violations = scan(SERVER_ROOT)
|
|
|
|
if (violations.length) {
|
|
console.error(`\n${violations.length} import(s) break the module boundary (MODULE_API.md §5.1):\n`)
|
|
for (const v of violations) {
|
|
console.error(` ${path.relative(MODULE_ROOT, v.file)}\n "${v.specifier}" — ${v.why}`)
|
|
}
|
|
console.error('')
|
|
process.exit(1)
|
|
}
|
|
|
|
console.log(`OK — no import escapes the module root (${SERVER_ROOT}).`)
|