diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 08cb264..f6cbcbf 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -16,6 +16,17 @@ # tree works right up until core moves a file, and the whole boundary is # worth exactly as much as this check is (§5.1). # +# • `server: check:bundle` — the release ships everything the entry point can +# reach. Every other job here runs against the whole repo, but a release is a +# SUBSET of it (release.yml assembles from the include list in +# `ci/bundle.json`), and nothing compared the two. On 2026-08-19 they +# disagreed: `server/commands/` arrived with the Teams cutover, the include +# list did not learn about it, and v1.0.0 installed and then died at the +# register stage on the operator's box with "Cannot find module +# './commands/guild.command'". Green here, broken there — because the subset +# only exists in the release. This asks, on the PR that adds the directory, +# whether the list still covers what index.js reaches. +# # • `client: check:externals` — the BUILT chunk has no bare imports left. That # failure is invisible in source: `import { useState } from 'react'` is # correct in every file, and whether it becomes core's React or a bare @@ -113,6 +124,9 @@ jobs: - name: Check the module boundary (MODULE_API.md §5.1) run: npm run check:imports --prefix server + - name: Check the release ships what the module requires + run: npm run check:bundle --prefix server + - name: Check the OpenAPI fragment is current (MODULE_API.md §2.8) run: npm run check:swagger --prefix server diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index 319088b..29fde15 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -301,6 +301,14 @@ jobs: # Stated as an INCLUDE list, not an exclude list. An exclude list ships # whatever it forgot: the day someone adds `server/tools/` with a scratch # credential in it, an exclude list packs it and nobody finds out. + # + # The list itself lives in `ci/bundle.json`, not here, because it has a + # second reader: `server/scripts/checkBundle.js` runs in PR checks and asks + # whether the list still covers everything `server/index.js` reaches. It + # was hardcoded in this file until v1.0.0 shipped without `server/commands/` + # — added by the Teams cutover, never added here — and the module died at + # the register stage on the operator's box. One declaration, two readers, + # so the next directory cannot go missing quietly. - name: Assemble the bundle if: ${{ steps.plan.outputs.release == 'true' }} run: | @@ -319,14 +327,15 @@ jobs: # The two fragments, and the licence the code is under — a bundle that # ships GPL code without its licence is not distributable. - cp swagger-fragment.json LICENSE.md README.md "$OUT/" + for f in $(jq -r '.root[]' ci/bundle.json); do + cp "$f" "$OUT/" + done # The server half, minus what never runs inside core's process. mkdir -p "$OUT/server" - for d in boot.js core.js index.js config data db model router utils; do + for d in $(jq -r '.server[]' ci/bundle.json); do cp -r "server/$d" "$OUT/server/" done - cp server/package.json "$OUT/server/" cp -r server/node_modules "$OUT/server/" # The client half is the BUILT chunk only. `client/src` is 5,000 lines @@ -354,6 +363,20 @@ jobs: console.log("bundle contents check: ok"); ' "$OUT" "$VERSION" + # ── And that it can actually LOAD ───────────────────────────────── + # + # The check above stats the paths `module.json` declares, which is a + # real question but a shallow one: v1.0.0 passed it and was still + # missing `server/commands/`, because a file reached only by a require + # inside `register()` is named nowhere in `module.json`. This resolves + # every relative require in the assembled tree and asserts the target is + # in it — asked of the artifact, so it also catches a copy that half + # failed or a list naming a path that has since moved. + # + # Run from the SOURCE tree (`server/scripts/` never ships) against the + # assembled bundle. + node server/scripts/checkBundle.js --bundle "$OUT" + tar -C dist -czf "dist/module-uo-${VERSION}.tar.gz" "module-uo-${VERSION}" rm -rf "$OUT" diff --git a/ci/bundle.json b/ci/bundle.json new file mode 100644 index 0000000..6e93918 --- /dev/null +++ b/ci/bundle.json @@ -0,0 +1,43 @@ +{ + "$comment": [ + "What a release copies into the bundle, declared ONCE. Read by .gitea/workflows/release.yml", + "when it assembles the tarball, and by server/scripts/checkBundle.js when CI asks whether", + "that list still covers everything the module's entry point can reach.", + "", + "This is an INCLUDE list on purpose (release.yml's header argues the case): an exclude list", + "ships whatever it forgot, so the day someone adds server/tools/ with a scratch credential", + "in it, an exclude list packs it and nobody finds out. The cost of that choice is that a new", + "top-level directory silently drops OUT of every release instead — which is exactly what", + "happened to server/commands/ between v0.3.0 and v1.0.0, and is why checkBundle.js exists.", + "", + "server[] entries are paths under server/; root[] and generated[] are paths under the module", + "root. node_modules is not listed: the release installs it with `npm ci --omit=dev` and copies", + "it separately, so it is not a checked-in path.", + "", + "generated[] ships but is not copied — release.yml writes module.json through jq to stamp the", + "released version into it, since the committed one is a floor rather than a record of the last", + "release. It is listed because server/index.js requires it, and a check that did not know it", + "ships would report the module's own manifest as missing from the bundle." + ], + "server": [ + "boot.js", + "commands", + "config", + "core.js", + "data", + "db", + "index.js", + "model", + "package.json", + "router", + "utils" + ], + "root": [ + "swagger-fragment.json", + "LICENSE.md", + "README.md" + ], + "generated": [ + "module.json" + ] +} diff --git a/server/package.json b/server/package.json index a7f0675..858544d 100644 --- a/server/package.json +++ b/server/package.json @@ -8,6 +8,7 @@ "scripts": { "test": "node --test --require ./test/_setup.js", "check:imports": "node scripts/checkImports.js", + "check:bundle": "node scripts/checkBundle.js", "swagger": "node scripts/swaggerFragment.js", "check:swagger": "node scripts/swaggerFragment.js --check" }, diff --git a/server/scripts/checkBundle.js b/server/scripts/checkBundle.js new file mode 100644 index 0000000..10811ba --- /dev/null +++ b/server/scripts/checkBundle.js @@ -0,0 +1,248 @@ +#!/usr/bin/env node +// ── Does the release actually ship everything the module needs? ──────────── +// +// `ci/bundle.json` says what a release copies. `server/index.js` says what the +// module requires. Nothing kept those two in agreement, and on 2026-08-19 they +// disagreed in production: `server/commands/` was added by the Teams cutover, +// the include list in release.yml was not updated, and v1.0.0 shipped without +// it. Every boot logged +// +// module "uo" failed to load — {"stage":"register","reason":"Cannot find +// module './commands/guild.command'"} +// +// and the module was dead on the operator's box. Nothing caught it: the PR +// checks install the module by copying the WHOLE repo into core, so they only +// ever exercised a tree that had the file. The release is the only place the +// subset exists, and the release had no check that the subset was complete. +// +// This script asks that question in the two places it can be asked: +// +// --check (PR checks) Every file reachable from the entry point by a +// relative require lives under something ci/bundle.json +// lists. Source-tree only, so it is fast and needs no +// assembled bundle — it fails on the PR that adds the +// directory, which is where the fix is cheapest. +// +// --bundle (release) Every relative specifier inside an ASSEMBLED bundle +// resolves to a file that is in it. Asked of the +// artifact rather than of the source, so it also +// catches a copy that half-failed, a list that names a +// path that has moved, and anything else between the +// declaration and the tarball. +// +// The two are deliberately not the same question. The first is about the list +// being right; the second is about the tarball being right. A release runs both. +// +// ── Why reachability, and not "require the entry point" ──────────────────── +// +// The obvious check — require the bundle's entry and see if it throws — does not +// work here, and the reason is in index.js's own header: its requires are inside +// `register()` because require order is load-bearing (`core.init(ctx)` has to run +// before anything under `router/` is required). So requiring the entry evaluates +// exactly one line, `require('./core')`, and reports success on a bundle missing +// every router it has. Calling `register()` for real would need a fake `ctx` +// complete enough to satisfy the whole module — which is what `test/` is for, and +// `test/` does not ship. Walking the requires statically asks the same question +// without needing either. + +const fs = require('fs') +const path = require('path') +const { stripCommentsAndTemplates } = require('./checkImports') + +const MODULE_ROOT = path.resolve(__dirname, '..', '..') +const SERVER_ROOT = path.join(MODULE_ROOT, 'server') + +// Only relative specifiers. A bare one is checkImports.js's question, not this +// one, and the two failures want different advice. +const RELATIVE = /(?:require\(|from\s+|import\()\s*['"](\.[^'"]+)['"]/g + +/** + * Resolve a relative specifier the way Node would, for the file cases that can + * appear here: an exact path, `+.js`/`+.json`, or a directory's `index.js`. + * + * Returns null when nothing exists — which is the finding, not an error. + */ +function resolveFile(fromDir, specifier) { + const base = path.resolve(fromDir, specifier) + const candidates = [base, `${base}.js`, `${base}.json`, path.join(base, 'index.js')] + for (const c of candidates) { + if (fs.existsSync(c) && fs.statSync(c).isFile()) return c + } + return null +} + +/** + * Every file reachable from `entry` by following relative requires, plus every + * specifier that resolved to nothing. + * + * Exported so the test can point it at fixtures — the same reason checkImports.js + * exports `scan`. A check that has never been shown to fail is a check nobody + * knows the state of, and this one is now load-bearing for every release. + */ +function reachable(entry) { + const seen = new Set() + const missing = [] + const queue = [entry] + + while (queue.length) { + const file = queue.shift() + if (seen.has(file)) continue + seen.add(file) + + // A .json dependency is a leaf: it is reached, it ships, and it has no + // requires of its own to follow. + if (file.endsWith('.json')) continue + + const source = stripCommentsAndTemplates(fs.readFileSync(file, 'utf8')) + for (const [, specifier] of source.matchAll(RELATIVE)) { + const target = resolveFile(path.dirname(file), specifier) + if (target) queue.push(target) + else missing.push({ file, specifier }) + } + } + + return { files: [...seen], missing } +} + +/** + * Everything ci/bundle.json says ends up in the bundle, as absolute paths: + * `server[]` relative to server/, `root[]` and `generated[]` relative to the + * module root. All three are equally "in the tarball" as far as a require is + * concerned — the only difference is how they get there. + */ +function declaredServerPaths(moduleRoot = MODULE_ROOT) { + const manifest = JSON.parse(fs.readFileSync(path.join(moduleRoot, 'ci', 'bundle.json'), 'utf8')) + return [ + ...manifest.server.map((p) => path.join(moduleRoot, 'server', p)), + ...(manifest.root || []).map((p) => path.join(moduleRoot, p)), + ...(manifest.generated || []).map((p) => path.join(moduleRoot, p)) + ] +} + +const covers = (declared, file) => + declared.some((d) => file === d || file.startsWith(d + path.sep)) + +/** + * --check: is ci/bundle.json's list sufficient for what the entry point reaches? + * + * Reports the top-level entry to ADD rather than the individual files, because + * that is the edit: the list is stated in top-level paths, and a new directory + * arrives with a dozen files in it. + */ +function checkDeclaration(moduleRoot = MODULE_ROOT) { + const serverRoot = path.join(moduleRoot, 'server') + const entry = path.join(serverRoot, 'index.js') + const { files, missing } = reachable(entry) + const declared = declaredServerPaths(moduleRoot) + + // Grouped by the entry that would have to be added, which is the top-level + // path under server/ — or, for the rare reachable file outside it, the path + // itself, since that one belongs in root[] instead. + const uncovered = new Map() + for (const file of files) { + if (covers(declared, file)) continue + const inServer = file.startsWith(serverRoot + path.sep) + const key = inServer + ? `server/${path.relative(serverRoot, file).split(path.sep)[0]}` + : path.relative(moduleRoot, file).split(path.sep).join('/') + if (!uncovered.has(key)) uncovered.set(key, []) + uncovered.get(key).push(file) + } + + return { uncovered, missing, reached: files.length } +} + +/** + * --bundle: does every relative specifier inside an assembled bundle resolve? + * + * Walks the bundle's own server tree rather than starting from the entry point, + * so a file that ships but is broken is caught too. + */ +function checkBundle(bundleRoot) { + const serverRoot = path.join(bundleRoot, 'server') + const missing = [] + const files = [] + + const walk = (dir) => { + for (const e of fs.readdirSync(dir, { withFileTypes: true })) { + const p = path.join(dir, e.name) + if (e.isDirectory()) { + // The installed dependency tree is npm's business, not this check's. + if (e.name !== 'node_modules') walk(p) + } else if (/\.(js|mjs|cjs)$/.test(e.name)) { + files.push(p) + } + } + } + walk(serverRoot) + + for (const file of files) { + const source = stripCommentsAndTemplates(fs.readFileSync(file, 'utf8')) + for (const [, specifier] of source.matchAll(RELATIVE)) { + if (!resolveFile(path.dirname(file), specifier)) missing.push({ file, specifier }) + } + } + + return { missing, scanned: files.length } +} + +module.exports = { reachable, resolveFile, checkDeclaration, checkBundle, declaredServerPaths } + +// Required by a test, or run as the check? Only the second one exits. +if (require.main !== module) return + +const bundleFlag = process.argv.indexOf('--bundle') + +if (bundleFlag !== -1) { + const root = process.argv[bundleFlag + 1] + if (!root) { + console.error('--bundle needs the path to an assembled bundle') + process.exit(2) + } + const { missing, scanned } = checkBundle(path.resolve(root)) + if (missing.length) { + console.error(`\nThe assembled bundle is incomplete — ${missing.length} require(s) resolve to nothing:\n`) + for (const m of missing) { + console.error(` ${path.relative(root, m.file)}\n requires "${m.specifier}" — not in the bundle`) + } + console.error('\nAdd the missing path to ci/bundle.json.\n') + process.exit(1) + } + console.log(`OK — every relative require in the bundle resolves (${scanned} files scanned).`) +} else { + const { uncovered, missing, reached } = checkDeclaration() + + if (missing.length) { + console.error(`\n${missing.length} require(s) resolve to nothing in the source tree:\n`) + for (const m of missing) { + console.error(` ${path.relative(MODULE_ROOT, m.file)}\n requires "${m.specifier}"`) + } + console.error('') + process.exit(1) + } + + if (uncovered.size) { + console.error(`\nci/bundle.json does not ship everything server/index.js reaches.\n`) + console.error('A release built from this list would install and then fail at the') + console.error('register stage with "Cannot find module", on the operator\'s box.\n') + for (const [key, files] of uncovered) { + console.error(` ${key} (${files.length} file${files.length === 1 ? '' : 's'} reachable)`) + for (const f of files.slice(0, 5)) console.error(` ${path.relative(MODULE_ROOT, f)}`) + if (files.length > 5) console.error(` … and ${files.length - 5} more`) + } + // server[] is written relative to server/, so name the entry to add rather + // than the path just displayed — they differ by exactly that prefix. + const toServer = [...uncovered.keys()].filter((k) => k.startsWith('server/')) + const toRoot = [...uncovered.keys()].filter((k) => !k.startsWith('server/')) + if (toServer.length) { + console.error(`\nAdd ${toServer.map((k) => `"${k.slice('server/'.length)}"`).join(', ')} to ci/bundle.json's server[].`) + } + if (toRoot.length) { + console.error(`\nAdd ${toRoot.map((k) => `"${k}"`).join(', ')} to ci/bundle.json's root[].`) + } + console.error('') + process.exit(1) + } + + console.log(`OK — ci/bundle.json ships every file server/index.js reaches (${reached} files).`) +} diff --git a/server/test/checkBundle.test.js b/server/test/checkBundle.test.js new file mode 100644 index 0000000..e46cced --- /dev/null +++ b/server/test/checkBundle.test.js @@ -0,0 +1,208 @@ +// The bundle check, checked. +// +// `scripts/checkBundle.js` exists because v1.0.0 shipped without +// `server/commands/` and died at the register stage on the operator's box. A +// check written in response to one bug is worth exactly as much as its coverage +// of that bug, so the first two tests below are that bug, in both modes: a list +// that has stopped covering what the entry point reaches, and a tarball with the +// file missing from it. +// +// **Every fixture is a template literal, and that is load-bearing** — the same +// reason checkImports.test.js gives. `scripts/checkImports.js` scans this +// directory too, so an ordinary quoted string holding a relative require would +// make this file fail that check. Templates are blanked by the stripper. + +const test = require('node:test') +const assert = require('node:assert') +const fs = require('node:fs') +const os = require('node:os') +const path = require('node:path') + +const { + reachable, + resolveFile, + checkDeclaration, + checkBundle, + declaredServerPaths +} = require('../scripts/checkBundle') + +/** + * Write a throwaway module tree: `files` under server/, `bundle` as its + * ci/bundle.json. Returns the module root. + */ +function fixture(files, bundle = { server: ['index.js'] }) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'module-uo-bundle-')) + for (const [name, source] of Object.entries(files)) { + const file = path.join(root, 'server', name) + fs.mkdirSync(path.dirname(file), { recursive: true }) + fs.writeFileSync(file, source) + } + fs.mkdirSync(path.join(root, 'ci'), { recursive: true }) + fs.writeFileSync(path.join(root, 'ci', 'bundle.json'), JSON.stringify(bundle)) + return root +} + +const cleanup = (root) => fs.rmSync(root, { recursive: true, force: true }) + +// ── The regression this script was written for ───────────────────────────── + +test('--check catches a directory the include list has stopped covering', () => { + const root = fixture( + { + 'index.js': `const g = require('./commands/guild.command')`, + 'commands/guild.command.js': `module.exports = {}` + }, + { server: ['index.js'] } // `commands` missing — exactly v1.0.0 + ) + try { + const { uncovered } = checkDeclaration(root) + assert.strictEqual(uncovered.size, 1) + assert.ok(uncovered.has('server/commands')) + } finally { + cleanup(root) + } +}) + +test('--bundle catches the file missing from an assembled tarball', () => { + const root = fixture({ 'index.js': `require('./commands/guild.command')` }) + try { + const { missing } = checkBundle(root) + assert.strictEqual(missing.length, 1) + assert.strictEqual(missing[0].specifier, './commands/guild.command') + } finally { + cleanup(root) + } +}) + +// ── It has to reach requires that are not at the top level ───────────────── + +test('follows requires written inside a function', () => { + // index.js requires inside `register()` because require order is load-bearing. + // A check that only saw file-scope requires would have missed the real bug. + const root = fixture( + { + 'index.js': `module.exports = function register(ctx) { const r = require('./router/a') }`, + 'router/a.js': `module.exports = {}` + }, + { server: ['index.js', 'router'] } + ) + try { + assert.strictEqual(checkDeclaration(root).uncovered.size, 0) + assert.strictEqual(checkBundle(root).missing.length, 0) + } finally { + cleanup(root) + } +}) + +test('follows requires transitively, not just one hop', () => { + const root = fixture( + { + 'index.js': `require('./a')`, + 'a.js': `require('./b')`, + 'b.js': `require('./deep/c')`, + 'deep/c.js': `module.exports = {}` + }, + { server: ['index.js', 'a.js', 'b.js'] } // `deep` missing + ) + try { + const { uncovered } = checkDeclaration(root) + assert.ok(uncovered.has('server/deep')) + } finally { + cleanup(root) + } +}) + +// ── Resolution has to match Node's, or it invents failures ───────────────── + +test('resolves a directory to its index.js', () => { + const root = fixture({ 'index.js': `require('./boot')`, 'boot/index.js': `module.exports = {}` }, + { server: ['index.js', 'boot'] }) + try { + assert.strictEqual(checkDeclaration(root).uncovered.size, 0) + } finally { + cleanup(root) + } +}) + +test('resolves a .json dependency, and does not try to parse it for requires', () => { + const root = fixture({ 'index.js': `require('./data/atlas.json')`, 'data/atlas.json': `{"a":1}` }, + { server: ['index.js', 'data'] }) + try { + const { uncovered, missing } = checkDeclaration(root) + assert.strictEqual(missing.length, 0) + assert.strictEqual(uncovered.size, 0) + } finally { + cleanup(root) + } +}) + +test('survives a require cycle', () => { + const root = fixture({ 'index.js': `require('./a')`, 'a.js': `require('./index')` }, + { server: ['index.js', 'a.js'] }) + try { + assert.strictEqual(checkDeclaration(root).uncovered.size, 0) + } finally { + cleanup(root) + } +}) + +test('a specifier that resolves to nothing is reported, not thrown', () => { + const root = fixture({ 'index.js': `require('./gone')` }) + try { + const { missing } = checkDeclaration(root) + assert.strictEqual(missing.length, 1) + assert.strictEqual(missing[0].specifier, './gone') + } finally { + cleanup(root) + } +}) + +test('prose describing a require is not a require', () => { + // The failure mode checkImports.js hit the first time it ran: index.js's own + // header explains why it must never require express, and comments in this + // repo name module paths constantly. + const root = fixture( + { 'index.js': `// this file used to require('./commands/gone')\nmodule.exports = 1` }, + { server: ['index.js'] } + ) + try { + assert.strictEqual(checkDeclaration(root).missing.length, 0) + } finally { + cleanup(root) + } +}) + +test('node_modules inside a bundle is npm\'s business, not this check\'s', () => { + const root = fixture({ + 'index.js': `module.exports = 1`, + 'node_modules/ws/index.js': `require('./lib/that-npm-owns')` + }) + try { + assert.strictEqual(checkBundle(root).missing.length, 0) + } finally { + cleanup(root) + } +}) + +// ── And the real repo, which is the check that actually gates a release ──── + +test('the real ci/bundle.json covers everything the real entry point reaches', () => { + const { uncovered, missing, reached } = checkDeclaration() + assert.deepStrictEqual([...uncovered.keys()], []) + assert.deepStrictEqual(missing, []) + assert.ok(reached > 1, 'the walk should reach more than the entry point itself') +}) + +test('every path ci/bundle.json declares exists', () => { + // A list naming a path that has moved packs nothing and says nothing — `cp` + // in the release would fail, but only after the tag had been pushed. + for (const p of declaredServerPaths()) { + assert.ok(fs.existsSync(p), `ci/bundle.json names ${p}, which does not exist`) + } +}) + +test('the entry point is reachable from the declared list', () => { + const entry = path.resolve(__dirname, '..', 'index.js') + assert.ok(reachable(entry).files.includes(entry)) + assert.ok(resolveFile(path.dirname(entry), './core')) +})