fix(kit): everything the acceptance run found — Phase 5 slice 3
A cold agent was given this repo and the documents it links to, and nothing else — no core source, no module-uo — and asked to build a module for a second game. It did, in one pass. The record is docs/modules/kit-acceptance.md; this is the repair list, plus the two things it recommended that were not defects. The one it could not find, because it had no core to render against: a module page built exactly as this kit teaches renders OUTSIDE the site. PublicLayout is the chrome, not the body. Core grew an opt-in `shell` prop for it (MODULE_API_VERSION 1.5.0, website#148); the template passes shell="narrow" and chapter 2 explains why you name a width and never a class. Fixed: - **F1, and the worst of them, because it lands in the first twenty minutes.** `npm run check:swagger` failed on a PRISTINE template on Windows: the check compared the committed fragment byte-for-byte and a default Windows clone is CRLF while the generator writes LF. The message blamed "the routes or their annotations". Now `template/.gitattributes` pins `eol=lf` and the comparison normalises line endings anyway — a check may only fail for the reason it names, and this one names a diagnosis. - **F3** — `.gitea/workflows/release.yml` carries `gitea.example.com` and `your-org/your-module` under a literal `# CHANGE THESE`, was not in the rename checklist, and `checkRenameSites.js` could not match it, so CI was silent by construction. Row added, pattern widened. (The agent reported both workflow flavours; only the Gitea one is affected — GitHub supplies its own variables. Corrected in the record.) The near-miss is kept in the check's comments and its suite: the obvious widening is `example\.com`, which fires on a fixture URL in checkImports.test.js. Every alternative has to be a string that cannot occur by accident, which is the same rule that made the id `examplegame`. - **F4** — the release bundle's include list was hardcoded, so adding `server/utils/` would have silently dropped it from every release while the bundle check stayed green. Inverted to an exclusion list, in both flavours, and run by hand because a release workflow never executes in CI. - **F5** — the annotation-quoting warning was wrong in both directions, and the correction is measured rather than reasoned. A backtick is harmless (the template's own description has two spans and they survive). A `"` is not, and it does not throw: `'A "quoted" status'` is silently TRUNCATED to `A "` while swagger-autogen prints Success and the error capture sees nothing. The only signal is check:swagger blaming your routes. - **F6** — `template/.gitignore`, so a copied template that is `git init`ed inherits ignore rules instead of nothing. - **F7** — the UI kit is eight exports across five rows, not seven. The contract said seven and this kit had faithfully carried the miscount out of it. Adopted, not defects: - Chapter 1 now says to run every check on the untouched copy first. That is what found F1; without a baseline the first failure is ambiguous forever. - The template ships the §2.7 self-check the agent wrote for itself. The rule has no CI in general — an outbound socket is not statically detectable — but a module can make a decidable claim about its own tree. Ported from its code with a header explaining how to NARROW it when a sidecar client arrives, since talking to your sidecar is the expected shape and is not what §2.7 forbids. The pin moves to website edge 4ad8b2b, the 1.5.0 bump, and template/module.json declares ^1.5.0 — so checkCoreApi's equality assertion still holds and the template uses a member that exists only at that ref and later. 32 server + 18 client template tests, 21 kit-script tests, all four checks green. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
22
template/.gitattributes
vendored
Normal file
22
template/.gitattributes
vendored
Normal file
@@ -0,0 +1,22 @@
|
||||
# Check every text file out with LF, on every platform.
|
||||
#
|
||||
# This exists because of a real failure, reported by the kit's acceptance run
|
||||
# (docs/modules/kit-acceptance.md, finding F1): on a default Windows clone,
|
||||
# `npm run check:swagger` failed on a PRISTINE, unedited template. The check
|
||||
# compares the committed swagger-fragment.json against what the generator writes;
|
||||
# the generator writes LF, and core.autocrlf had handed the reader CRLF. The
|
||||
# message blamed "the routes or their annotations", which is the first command the
|
||||
# kit tells a reader to run telling them a false thing about their own work.
|
||||
#
|
||||
# The check itself now normalises line endings before comparing, so this file is
|
||||
# the belt to that pair of braces: it also stops a CRLF blob ever being COMMITTED
|
||||
# by a reader who copies this template, which would break the same check for
|
||||
# everyone who cloned their repo afterwards.
|
||||
#
|
||||
# It travels with the template on purpose — a copied module directory keeps its
|
||||
# own attributes, and this is one of the things the copier should not have to know.
|
||||
* text=auto eol=lf
|
||||
|
||||
# Nothing here is binary today. If your module ships an image or a font, mark it,
|
||||
# because `text=auto` guesses and a wrong guess corrupts the file:
|
||||
# *.png binary
|
||||
@@ -151,12 +151,23 @@ jobs:
|
||||
|
||||
# The server half, minus everything that never runs inside core's
|
||||
# process: no `test/`, no `scripts/`, no `swagger/`.
|
||||
#
|
||||
# An EXCLUSION list, not an include list, and that is the whole point.
|
||||
# This was `for d in boot.js core.js index.js db model router`, which
|
||||
# meant adding `server/utils/` — an ordinary thing to do — silently
|
||||
# dropped it from every release: the bundle check below only resolves
|
||||
# the five paths module.json names, so nothing failed here, and the
|
||||
# module died on an operator's box as a `startup_failed` row instead
|
||||
# (docs/modules/kit-acceptance.md, F4). Excluding is the safe default
|
||||
# because the failure mode inverts: forget to exclude something and you
|
||||
# ship a harmless extra file, rather than omitting a required one.
|
||||
mkdir -p "$OUT/server"
|
||||
for d in boot.js core.js index.js db model router; do
|
||||
cp -r "server/$d" "$OUT/server/"
|
||||
for e in server/*; do
|
||||
case "$(basename "$e")" in
|
||||
test|scripts|swagger|package-lock.json) continue ;;
|
||||
esac
|
||||
cp -r "$e" "$OUT/server/"
|
||||
done
|
||||
cp server/package.json "$OUT/server/"
|
||||
[ -d server/node_modules ] && cp -r server/node_modules "$OUT/server/" || true
|
||||
|
||||
# The client half is the BUILT chunk only. `client/src` is source an
|
||||
# operator has no use for and core will never read.
|
||||
|
||||
19
template/.github/workflows/release.yml
vendored
19
template/.github/workflows/release.yml
vendored
@@ -135,12 +135,23 @@ jobs:
|
||||
|
||||
# The server half, minus everything that never runs inside core's
|
||||
# process: no `test/`, no `scripts/`, no `swagger/`.
|
||||
#
|
||||
# An EXCLUSION list, not an include list, and that is the whole point.
|
||||
# This was `for d in boot.js core.js index.js db model router`, which
|
||||
# meant adding `server/utils/` — an ordinary thing to do — silently
|
||||
# dropped it from every release: the bundle check below only resolves
|
||||
# the five paths module.json names, so nothing failed here, and the
|
||||
# module died on an operator's box as a `startup_failed` row instead
|
||||
# (docs/modules/kit-acceptance.md, F4). Excluding is the safe default
|
||||
# because the failure mode inverts: forget to exclude something and you
|
||||
# ship a harmless extra file, rather than omitting a required one.
|
||||
mkdir -p "$OUT/server"
|
||||
for d in boot.js core.js index.js db model router; do
|
||||
cp -r "server/$d" "$OUT/server/"
|
||||
for e in server/*; do
|
||||
case "$(basename "$e")" in
|
||||
test|scripts|swagger|package-lock.json) continue ;;
|
||||
esac
|
||||
cp -r "$e" "$OUT/server/"
|
||||
done
|
||||
cp server/package.json "$OUT/server/"
|
||||
[ -d server/node_modules ] && cp -r server/node_modules "$OUT/server/" || true
|
||||
|
||||
# The client half is the BUILT chunk only.
|
||||
mkdir -p "$OUT/client/dist"
|
||||
|
||||
36
template/.gitignore
vendored
Normal file
36
template/.gitignore
vendored
Normal file
@@ -0,0 +1,36 @@
|
||||
# The template's own ignore rules, so they travel with a copy.
|
||||
#
|
||||
# The kit repo's root .gitignore covers these paths too, but that file stays
|
||||
# behind: copy `template/` out, `git init`, and you inherit nothing — `node_modules/`
|
||||
# included. Reported by the acceptance run (docs/modules/kit-acceptance.md, F6).
|
||||
|
||||
# dependencies
|
||||
node_modules/
|
||||
|
||||
# ── The built client chunk ────────────────────────────────────────────────
|
||||
#
|
||||
# Ignored HERE and shipped in the RELEASE, which is not a contradiction: a module
|
||||
# is installed prebuilt (an operator never builds anything), but the artifact is
|
||||
# built by CI from the source next to it, and a chunk committed by hand goes stale
|
||||
# beside fresh source without anything saying so.
|
||||
#
|
||||
# Your release workflow builds it before packing the bundle. If you would rather
|
||||
# commit it, delete this line and accept that you now have to remember.
|
||||
client/dist/
|
||||
|
||||
# env / secrets — a module's env vars are the operator's, never the repo's
|
||||
.env
|
||||
*.env
|
||||
!.env.example
|
||||
|
||||
# release staging, produced by .gitea/workflows/release.yml
|
||||
/dist/
|
||||
*.tar.gz
|
||||
|
||||
# logs / os / editor
|
||||
*.log
|
||||
npm-debug.log*
|
||||
.DS_Store
|
||||
Thumbs.db
|
||||
.vscode/
|
||||
.idea/
|
||||
@@ -35,7 +35,7 @@ server/
|
||||
client/
|
||||
vite.config.js the library build: anchored aliases, external: []
|
||||
src/entry.jsx registers routes and nav at evaluation time
|
||||
src/core.js what core hands you: the seven-member UI kit
|
||||
src/core.js what core hands you: the UI kit (eight exports)
|
||||
src/shim/ the four shared dependencies, re-exported from core
|
||||
src/routes/public/ the page
|
||||
scripts/checkExternals.js asks the BUILT chunk whether a bare import survived
|
||||
@@ -103,6 +103,7 @@ backticking table names**.
|
||||
| File | What to change |
|
||||
| --- | --- |
|
||||
| `module.json` | `id`, `name`, `version`, the `mounts` prefix, `capabilities` |
|
||||
| `.gitea/workflows/release.yml` | `GITEA_HOST` and `REPO`, under the `# CHANGE THESE` banner — the only two, and they are wrong until you do. (The `.github/` flavour needs nothing: GitHub supplies `GITHUB_REPOSITORY` and friends.) |
|
||||
| `server/package.json` | package `name` and `description` |
|
||||
| `server/core.js` | the message every accessor throws |
|
||||
| `server/boot.js` | the placeholder world name |
|
||||
|
||||
@@ -45,14 +45,14 @@ if (createElement !== rg.react.createElement || createRoot !== rg.reactDom.creat
|
||||
)
|
||||
}
|
||||
|
||||
// The curated kit (§3.4). Seven members, and it is CLOSED: layout, headings, the
|
||||
// The curated kit (§3.4). Eight exports, and it is CLOSED: layout, headings, the
|
||||
// three data-page states, the fetch hook, and read-only access to the session and
|
||||
// the site's settings. Anything else your pages need — tables, tabs, an editor —
|
||||
// you bundle yourself, in a `components/` directory of your own.
|
||||
//
|
||||
// Closed is a real constraint and it is the price of the boundary being worth
|
||||
// anything: adding a member is a minor `MODULE_API_VERSION` bump, and changing a
|
||||
// kit component's props is a major one. Use them, though. A module page that
|
||||
// anything: adding a member is a minor `MODULE_API_VERSION` bump, and changing an
|
||||
// existing prop on a kit component is a major one. Use them, though. A module page that
|
||||
// ships its own layout is a page that stops looking like the site it is installed
|
||||
// in, and drifts further every time core changes.
|
||||
export const {
|
||||
|
||||
@@ -3,13 +3,24 @@
|
||||
// An ordinary React component. Nothing about being inside a module changes how
|
||||
// you write one — the only differences are where React comes from (core, via the
|
||||
// aliases in vite.config.js, so the import below looks completely normal and is
|
||||
// not) and where the chrome comes from (`../../core.js`, the seven-member kit).
|
||||
// not) and where the chrome comes from (`../../core.js`, the shared UI kit).
|
||||
//
|
||||
// **Render `PublicLayout` yourself.** Core wraps your public routes in its
|
||||
// maintenance gate and nothing else, so a page that omits the layout renders
|
||||
// bare — no header, no footer, no site chrome — which looks like a bug and is
|
||||
// the contract (§3.3). Admin and player routes are the other way round: core
|
||||
// wraps those in their layouts for you.
|
||||
//
|
||||
// **And pass a `shell`.** The layout is the chrome; `shell` is the body — the
|
||||
// centred column, the vertical padding, and the thing that holds the footer at
|
||||
// the bottom of the viewport. Without it your content starts hard against the
|
||||
// left edge of the window and the footer rides up underneath it, which reads as
|
||||
// a CSS bug in your module and is not one. Widths are 'narrow', 'mid' and
|
||||
// 'wide'; name a width, never a class, because the classes belong to core's
|
||||
// stylesheet and it is free to rename them (§3.4, MODULE_API_VERSION 1.5.0).
|
||||
//
|
||||
// This is here because the kit's acceptance run got it wrong by following the
|
||||
// kit: a module built to the letter of chapter 2 rendered outside the site.
|
||||
|
||||
import { ErrorState, Loading, PageHeader, PublicLayout, useAsync } from '../../core.js'
|
||||
import api from '../../api.js'
|
||||
@@ -34,7 +45,7 @@ export default function WorldStatus() {
|
||||
const { data, loading, error } = useAsync(() => api.world.status(), [])
|
||||
|
||||
return (
|
||||
<PublicLayout>
|
||||
<PublicLayout shell="narrow">
|
||||
<PageHeader
|
||||
title="World status"
|
||||
subtitle="What the game server last told us about itself"
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
"id": "examplegame",
|
||||
"name": "Example Game",
|
||||
"version": "0.1.0",
|
||||
"coreApi": "^1.4.0",
|
||||
"coreApi": "^1.5.0",
|
||||
"server": "server/index.js",
|
||||
"client": { "entry": "client/dist/entry.js" },
|
||||
"schema": "server/db/schema.sql",
|
||||
|
||||
@@ -19,13 +19,21 @@
|
||||
// fragment is generated from — `npm run swagger` parses this file (§2.8). Two
|
||||
// rules that cost this project real time:
|
||||
//
|
||||
// • swagger-autogen reads these as JavaScript literals it evaluates. It
|
||||
// re-quotes `"` and a backtick to `'` first, so either one inside a
|
||||
// single-quoted description ends the string early — and when it cannot parse
|
||||
// an annotation it drops that annotation, prints an error, and then reports
|
||||
// success. Use a typographic apostrophe (’) in prose. `swaggerFragment.js`
|
||||
// captures those errors and makes them fatal, which is the only reason you
|
||||
// will find out.
|
||||
// • swagger-autogen reads these as JavaScript literals it evaluates, so a
|
||||
// QUOTE CHARACTER inside a single-quoted description ends the string early.
|
||||
// Both `'` and `"` — use a typographic apostrophe (’) in prose, and rewrite
|
||||
// a quoted phrase without the quotes. A backtick is fine: Markdown spans like
|
||||
// `online: false` below survive verbatim, and the fragment shows them.
|
||||
//
|
||||
// **The failure is silent, and this is the part worth remembering.** It is
|
||||
// not always a parse error you get told about. A `"` in the middle of a
|
||||
// description truncates the value at that character — `'A "quoted" status'`
|
||||
// becomes `A "` — while swagger-autogen prints `Success` in green and the
|
||||
// error capture below sees nothing to capture, because nothing threw. The
|
||||
// only signal is `npm run check:swagger` reporting the fragment stale, whose
|
||||
// message will blame your routes. When it does and your routes did not
|
||||
// change, look for a quote in an annotation before you look anywhere else.
|
||||
// (Measured, not inferred: docs/modules/kit-acceptance.md, F5.)
|
||||
// • A `\'` escape is valid JavaScript and wrong here: the annotation is never
|
||||
// evaluated as JS by the reader, so Swagger UI renders the backslash.
|
||||
|
||||
|
||||
@@ -234,7 +234,17 @@ async function main() {
|
||||
process.stderr.write('\nswagger-fragment.json is missing. Run `npm run swagger`.\n')
|
||||
process.exit(1)
|
||||
}
|
||||
if (fs.readFileSync(FRAGMENT, 'utf8') !== json) {
|
||||
// Compared with line endings normalised, and that is not fussiness. A default
|
||||
// Windows clone checks this file out as CRLF while the generator above writes
|
||||
// LF, so a byte comparison failed on a PRISTINE template and told the reader
|
||||
// their routes had changed — the kit's acceptance run lost ten minutes to it
|
||||
// before reaching for `od -c` (docs/modules/kit-acceptance.md, F1). A check may
|
||||
// only fail for the reason it names; this one names a diagnosis, so it has to
|
||||
// be right about it. `.gitattributes` stops the CRLF from arriving in the first
|
||||
// place, and this stops it mattering if it does.
|
||||
const lf = (s) => s.replace(/\r\n/g, '\n')
|
||||
|
||||
if (lf(fs.readFileSync(FRAGMENT, 'utf8')) !== lf(json)) {
|
||||
process.stderr.write(
|
||||
'\nswagger-fragment.json is STALE — the routes or their annotations changed and it was not\n' +
|
||||
'regenerated. Run `npm run swagger` and commit the result. Core merges this file verbatim,\n' +
|
||||
|
||||
124
template/server/test/noGameConnection.test.js
Normal file
124
template/server/test/noGameConnection.test.js
Normal file
@@ -0,0 +1,124 @@
|
||||
// ── §2.7's last rule, given the CI it does not have ───────────────────────
|
||||
//
|
||||
// `book/02-website-module.md` is explicit that "the website process never opens a
|
||||
// connection to a game server" is the **one boundary rule with no CI behind it**:
|
||||
// an outbound socket is not statically detectable the way an internal `require`
|
||||
// is, so in general the rule is held up by review and by understanding it.
|
||||
//
|
||||
// True of the general case, and not a reason to check nothing. A module can state
|
||||
// a narrower, completely decidable property about **itself**, and this one says:
|
||||
// the shipped server half references no networking primitive at all. Everything
|
||||
// it knows arrives from its own tables, which its sidecar writes.
|
||||
//
|
||||
// Adopted from the kit's acceptance run (`docs/modules/kit-acceptance.md`), where
|
||||
// a reader building a Rust module wrote it unprompted after reading that the rule
|
||||
// had no CI — and observed that for Rust in particular, which ships RCON over
|
||||
// WebSocket, `new WebSocket(rconUrl)` in `boot.js` is about ten lines away.
|
||||
//
|
||||
// ── WHEN YOU ADD A SIDECAR CLIENT, NARROW THIS. DO NOT DELETE IT. ─────────
|
||||
//
|
||||
// Talking to *your sidecar* over HTTP is the expected shape and is not what §2.7
|
||||
// forbids — the rule is about the **game server**. So the moment your module
|
||||
// grows, say, `server/sidecarClient.js`, this test starts failing correctly and
|
||||
// the fix is to allow that one file:
|
||||
//
|
||||
// const MAY_OPEN_SOCKETS = new Set(['sidecarClient.js'])
|
||||
//
|
||||
// and keep the rest of the tree under the ban. What you get for that is a test
|
||||
// that names the *one* file allowed to reach the network — which is exactly the
|
||||
// file a reviewer should be reading closely, and exactly the place a game-server
|
||||
// URL would appear if the rule were ever broken.
|
||||
//
|
||||
// Scope: SHIPPED code only. `test/` and `scripts/` never run inside core's process.
|
||||
|
||||
const test = require('node:test')
|
||||
const assert = require('node:assert')
|
||||
const fs = require('node:fs')
|
||||
const path = require('node:path')
|
||||
|
||||
const SERVER_ROOT = path.resolve(__dirname, '..')
|
||||
const NOT_SHIPPED = new Set(['test', 'scripts', 'node_modules', 'swagger'])
|
||||
|
||||
/** Every shipped `.js` file under `server/`. */
|
||||
function shippedFiles(dir = SERVER_ROOT, out = []) {
|
||||
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
|
||||
if (entry.isDirectory()) {
|
||||
if (dir === SERVER_ROOT && NOT_SHIPPED.has(entry.name)) continue
|
||||
if (entry.name === 'node_modules') continue
|
||||
shippedFiles(path.join(dir, entry.name), out)
|
||||
} else if (entry.isFile() && entry.name.endsWith('.js')) {
|
||||
out.push(path.join(dir, entry.name))
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
/**
|
||||
* Blank comments, so prose ABOUT the rule does not trip the rule.
|
||||
*
|
||||
* This file is itself the proof that it is needed: the paragraphs above say
|
||||
* "WebSocket" several times. `scripts/checkImports.js` documents hitting exactly
|
||||
* this on its own documentation, and it is the third time in this project's
|
||||
* history that a boundary check has failed on the text explaining it.
|
||||
*
|
||||
* Blanked rather than deleted, so line numbers in a failure still point at the
|
||||
* right line.
|
||||
*/
|
||||
function stripComments(src) {
|
||||
return src
|
||||
.replace(/\/\*[\s\S]*?\*\//g, (m) => m.replace(/[^\n]/g, ' '))
|
||||
.replace(/^[ \t]*\/\/.*$/gm, '')
|
||||
}
|
||||
|
||||
// Each is a way a Node process opens a socket. Matched as identifiers, so a
|
||||
// column named `websocket_url` inside a SQL string would not fire.
|
||||
const NETWORKING = [
|
||||
/\brequire\(\s*['"](?:node:)?(?:net|tls|dgram|http|https|http2)['"]\s*\)/,
|
||||
/\bfrom\s+['"](?:node:)?(?:net|tls|dgram|http|https|http2)['"]/,
|
||||
/\brequire\(\s*['"](?:ws|socket\.io-client|undici|axios|node-fetch|got)['"]\s*\)/,
|
||||
/\bnew\s+WebSocket\b/,
|
||||
/\bfetch\s*\(/,
|
||||
/\bXMLHttpRequest\b/,
|
||||
/\bEventSource\b/,
|
||||
]
|
||||
|
||||
test('no shipped file references a networking primitive (§2.7)', () => {
|
||||
const offenders = []
|
||||
for (const file of shippedFiles()) {
|
||||
const code = stripComments(fs.readFileSync(file, 'utf8'))
|
||||
for (const pattern of NETWORKING) {
|
||||
if (pattern.test(code)) {
|
||||
offenders.push(`${path.relative(SERVER_ROOT, file)} matches ${pattern}`)
|
||||
}
|
||||
}
|
||||
}
|
||||
assert.deepStrictEqual(
|
||||
offenders,
|
||||
[],
|
||||
'the website process must never open a connection to a game server. If this is ' +
|
||||
'your sidecar client, allow that one file rather than removing the check — see ' +
|
||||
`the header of this file.\n ${offenders.join('\n ')}`,
|
||||
)
|
||||
})
|
||||
|
||||
test('the check can actually fail — it is pointed at a real violation', () => {
|
||||
// A check that has never been shown to fail is a check nobody knows the state
|
||||
// of. This is the game-server dial the rule exists to stop.
|
||||
const violation = "const socket = new WebSocket('ws://10.0.0.5:28016/' + rconPassword)"
|
||||
assert.ok(
|
||||
NETWORKING.some((p) => p.test(stripComments(violation))),
|
||||
'the guard would not have caught a direct game-server dial',
|
||||
)
|
||||
})
|
||||
|
||||
test('prose describing the rule does not trip it', () => {
|
||||
const prose = [
|
||||
'// A game shipping RCON over WebSocket means a module COULD write',
|
||||
"// const s = new WebSocket(url); require('net')",
|
||||
'// in about ten lines. It must not.',
|
||||
'const x = 1',
|
||||
].join('\n')
|
||||
for (const pattern of NETWORKING) {
|
||||
assert.ok(!pattern.test(stripComments(prose)), `${pattern} fired on a comment`)
|
||||
}
|
||||
})
|
||||
Reference in New Issue
Block a user