Files
Module-uo/server/scripts/checkImports.js
wtclaude 47809854ef
All checks were successful
PR Checks / server-tests (pull_request) Successful in 12s
PR Checks / client-build (pull_request) Successful in 15s
fix(module): ask Node whether a specifier is a builtin
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>
2026-08-11 01:36:57 -05:00

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}).`)