fix(module): ask Node whether a specifier is a builtin
All checks were successful
PR Checks / server-tests (pull_request) Successful in 12s
PR Checks / client-build (pull_request) Successful in 15s

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>
This commit is contained in:
2026-08-11 01:36:57 -05:00
committed by Claude
parent 5d7668d5ea
commit 47809854ef
2 changed files with 27 additions and 7 deletions

View File

@@ -28,19 +28,23 @@
// because a test that reaches into core's tree is a test that passes on this // because a test that reaches into core's tree is a test that passes on this
// machine and nowhere else. // machine and nowhere else.
// //
// Run over the SERVER half. The client half's equivalent is its Vite build: // Run over the SERVER half. The client half's equivalents are its Vite build,
// the four shared dependencies are `external`, and anything else that stays a // which fails if a shared dependency resolves into node_modules, and
// bare import in the emitted chunk is unresolvable in the browser. // client/scripts/checkExternals.js, which asks the built chunk whether any bare
// specifier survived.
const fs = require('fs') const fs = require('fs')
const path = require('path') const path = require('path')
const { builtinModules } = require('module') // 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 MODULE_ROOT = path.resolve(__dirname, '..', '..')
const SERVER_ROOT = path.join(MODULE_ROOT, 'server') const SERVER_ROOT = path.join(MODULE_ROOT, 'server')
const BUILTINS = new Set([...builtinModules, ...builtinModules.map((m) => `node:${m}`)])
// Dependencies this half is allowed to resolve for itself. Empty, and that is // 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 // 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 // entry here is a real decision — it becomes a package an operator's install
@@ -148,7 +152,10 @@ function scan(root, moduleRoot = MODULE_ROOT, { shipped = isShipped, dev = devDe
? specifier.split('/').slice(0, 2).join('/') ? specifier.split('/').slice(0, 2).join('/')
: specifier.split('/')[0] : specifier.split('/')[0]
const allowed = ALLOWED_PACKAGES.has(pkg) || (!shipped(file) && dev.has(pkg)) const allowed = ALLOWED_PACKAGES.has(pkg) || (!shipped(file) && dev.has(pkg))
if (!BUILTINS.has(specifier) && !BUILTINS.has(pkg) && !allowed) { // 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?' }) violations.push({ file, specifier, why: 'undeclared bare specifier — should this come from ctx?' })
} }
} }

View File

@@ -81,6 +81,19 @@ test('allows node builtins anywhere, with or without the node: prefix', () => {
) )
}) })
test('allows node:test, which older Node versions omit from builtinModules', () => {
// The first CI run failed on exactly this and on nothing else: `builtinModules`
// omits `test` on Node 20 and includes it on Node 24, so every test file in
// this suite was reported as breaking the module boundary. The check asks
// Node (`isBuiltin`) rather than rebuilding the list, and treats the `node:`
// prefix as sufficient on its own — a prefixed specifier can never resolve to
// a package, whatever the running version enumerates.
assert.deepStrictEqual(
scanFixture({ 'a.js': `require('node:test'); require('node:test/reporters')` }),
[],
)
})
test('catches ESM and dynamic forms, not only require()', () => { test('catches ESM and dynamic forms, not only require()', () => {
const found = scanFixture({ const found = scanFixture({
'a.js': [`import db from '../../core/db.js'`, `const x = await import('../../core/other.js')`].join('\n'), 'a.js': [`import db from '../../core/db.js'`, `const x = await import('../../core/other.js')`].join('\n'),