Merge pull request 'fix(release): ship server/commands, and check that the bundle is complete' (#18) from fix/bundle-ships-commands into main

Reviewed-on: #18

.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
 

.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"
 

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"
+  ]
+}

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"
   },

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

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'))
+})