From f7bb3d912ef1288724d893bd94791f0b53a5122b Mon Sep 17 00:00:00 2001 From: wtclaude Date: Tue, 11 Aug 2026 21:27:52 -0500 Subject: [PATCH 1/2] fix(db): own the two settings seeds, and repair the protocol-3 one-shot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Core seeded `game_account_signup` and `uo_link_protocol_3_migrated`, two keys that name a game concept. That made core's schema declare a module's settings, which is the structural half of what Phase 3 removes (MODULE_SYSTEM.md §2.7.1, slice 4). Both INSERTs move here. The keys are deliberately unchanged: they are live rows on every existing install and renaming one silently resets an operator's choice to the default. The marker is not just a tidy-up. It and the `UPDATE uo_link_config SET protocol = 3` it makes one-shot were adjacent in core's schema.sql until slice 1 moved the UPDATE here and left the INSERT behind — and the two files do not run together: core's schema is replayed in full before any module fragment. So the marker existed before the UPDATE ever read it, the NOT EXISTS guard was false on every boot of an upgraded install, and the migration could never fire. An install carrying a protocol-2 row would have stayed pinned at 2 against a v3 sidecar, 409ing every REST call — the exact failure the migration prevents. Latent rather than live: it bites only an install that first boots a post-slice-1 build while already holding a uo_link_config row, and `edge` has not cut over. `schemaFragment.test.js` asserts the order, plus the fragment rules core validates at load time (leading-verb allowlist, IF NOT EXISTS, grandfathered table prefixes) — restated here for the same reason manifest.test.js restates the manifest rules. Its statement splitter is a character walk, because a comment in this file contains quotes. Co-Authored-By: Claude --- server/db/purge.sql | 6 +- server/db/schema.sql | 29 +++++- server/test/schemaFragment.test.js | 157 +++++++++++++++++++++++++++++ 3 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 server/test/schemaFragment.test.js diff --git a/server/db/purge.sql b/server/db/purge.sql index 7215d90..7b6cc48 100644 --- a/server/db/purge.sql +++ b/server/db/purge.sql @@ -21,7 +21,11 @@ -- `notification_subs` rows for `shard.*` streams and `announce_job_legs` rows -- with leg `towncrier` belong to core's tables, and a module does not delete -- from those — core prunes them when it drops the registrations, which it can --- do because it knows which registrant owned what. +-- do because it knows which registrant owned what. The two `settings` rows +-- schema.sql seeds (`game_account_signup`, `uo_link_protocol_3_migrated`) are +-- the same case with an extra reason: the second is a one-shot MIGRATION +-- marker, and deleting it would re-arm a protocol bump against tables this +-- file has just dropped. DROP TABLE IF EXISTS `shard_atlas_pending`; DROP TABLE IF EXISTS `shard_atlas_meta`; diff --git a/server/db/schema.sql b/server/db/schema.sql index 89d4bf5..ae263e4 100644 --- a/server/db/schema.sql +++ b/server/db/schema.sql @@ -614,4 +614,31 @@ ALTER TABLE uo_link_config MODIFY COLUMN protocol INT NOT NULL DEFAULT 3; -- uo_link_config row yet) it is simply written with nothing to update. UPDATE uo_link_config SET protocol = 3 WHERE id = 1 AND protocol < 3 - AND NOT EXISTS (SELECT 1 FROM settings WHERE `key` = 'uo_link_protocol_3_migrated'); \ No newline at end of file + AND NOT EXISTS (SELECT 1 FROM settings WHERE `key` = 'uo_link_protocol_3_migrated'); +-- **The marker must be written HERE, not in core.** These two statements were +-- adjacent in core's schema.sql before the extraction; slice 1 moved the UPDATE +-- and left the INSERT behind, and the two files do not run at the same time — +-- core's schema is replayed in full BEFORE any module fragment (MODULE_API.md +-- §2.6). So the marker existed before the UPDATE ever read it, the NOT EXISTS +-- was true on the first boot of a fresh install and false on every boot of an +-- upgraded one, and the one-shot could never fire. An install carrying a +-- protocol-2 row would have stayed pinned at 2 against a v3 sidecar — every +-- REST call 409, which is precisely the failure this migration exists to +-- prevent. Latent rather than live: it only bites an install that first boots a +-- post-slice-1 build while already holding a uo_link_config row, and `edge` has +-- not cut over yet. +INSERT IGNORE INTO settings (`key`, value) VALUES ('uo_link_protocol_3_migrated', '1'); + +-- ── Settings rows this module owns ───────────────────────────────────────── +-- +-- Both keys predate the module system and both name a game concept, so core +-- seeding them made core's schema declare a module's settings — the structural +-- half of what Phase 3 removes (MODULE_SYSTEM.md §2.7.1, slice 4). The KEYS are +-- deliberately unchanged: they are live rows on every existing install, and +-- renaming one would silently reset an operator's choice to the default. +-- +-- INSERT IGNORE, so an install that already carries the row keeps its value and +-- only a database that has never seen the key gets the default. Nothing in core +-- reads either one; `game_account_signup` is read through ctx.settings by +-- server/utils/gameSignup.js, which owns the policy. +INSERT IGNORE INTO settings (`key`, value) VALUES ('game_account_signup', 'disabled'); \ No newline at end of file diff --git a/server/test/schemaFragment.test.js b/server/test/schemaFragment.test.js new file mode 100644 index 0000000..cdf29c0 --- /dev/null +++ b/server/test/schemaFragment.test.js @@ -0,0 +1,157 @@ +// `schema.sql` is replayed by core on EVERY boot, and core validates it before +// anything mounts. The rules are core's (MODULE_API.md §2.6, loader.js), and are +// restated here for the same reason `manifest.test.js` restates the manifest +// rules: a mistake should fail in this repo's CI, which can say what is wrong, +// rather than on an install, where the symptom is a module that is simply absent. +// +// The test this file exists for is the ORDER one. Two statements that read each +// other were adjacent in core's schema.sql until slice 1 moved one of them here +// and left the other behind — and because core's schema is replayed in full +// before any module fragment, the marker was written before the migration that +// reads it and the one-shot could never fire. Nothing caught it: both files were +// individually valid SQL, both replayed cleanly, and the failure only shows on an +// upgraded install talking to a real sidecar. An assertion about order is the +// only thing that would have. + +const test = require('node:test') +const assert = require('node:assert') +const fs = require('node:fs') +const path = require('node:path') + +const SCHEMA = path.join(__dirname, '..', 'db', 'schema.sql') +const sql = fs.readFileSync(SCHEMA, 'utf8') + +/** + * Split into statements the way core's `utils/sqlStatements.js` does: a + * character walk, not a regexp. + * + * Comments are stripped before quotes are considered, because a comment may + * contain quotes — line 508 of this very file is `-- '' when randomised per + * activation`, and a stripper that opened a string there would swallow the rest + * of the file. The reverse case (a `--` inside a string literal) is handled by + * the same walk, since a quote opened outside a comment stays open. + */ +function splitStatements(text) { + const out = [] + let buf = '' + let quote = null + for (let i = 0; i < text.length; i++) { + const c = text[i] + if (quote) { + buf += c + if (c === '\\') { + buf += text[++i] ?? '' + } else if (c === quote) { + quote = null + } + continue + } + if (c === '-' && text[i + 1] === '-') { + while (i < text.length && text[i] !== '\n') i++ + buf += '\n' + continue + } + if (c === "'" || c === '"' || c === '`') { + quote = c + buf += c + continue + } + if (c === ';') { + if (buf.trim()) out.push(buf.trim()) + buf = '' + continue + } + buf += c + } + if (buf.trim()) out.push(buf.trim()) + return out +} + +const statements = splitStatements(sql) + +// Core's leading-verb allowlist. Not a DROP denylist: this file replays on every +// boot, so a TRUNCATE or DELETE would empty a table at each restart. +const ALLOWED = ['CREATE', 'ALTER', 'INSERT', 'UPDATE'] + +test('the splitter survives a comment that contains quotes', () => { + const parts = splitStatements("SELECT 1; -- '' a quote in a comment\nSELECT 2;") + assert.deepEqual( + parts.map((s) => s.trim().split('\n')[0].trim()), + ['SELECT 1', 'SELECT 2'], + ) +}) + +test('the splitter does not treat a -- inside a string as a comment', () => { + const parts = splitStatements("INSERT INTO t VALUES ('a--b');") + assert.equal(parts.length, 1) + assert.match(parts[0], /'a--b'/) +}) + +test('every statement leads with a verb core allows', () => { + for (const statement of statements) { + const verb = statement.trim().split(/\s+/)[0].toUpperCase() + assert.ok(ALLOWED.includes(verb), `statement leads with "${verb}": ${statement.slice(0, 70)}`) + } +}) + +test('every CREATE TABLE is IF NOT EXISTS', () => { + for (const statement of statements) { + if (!/^CREATE\s+TABLE/i.test(statement)) continue + assert.match(statement, /^CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS/i, statement.slice(0, 70)) + } +}) + +test('every table this fragment declares is prefixed shard_ or uo_link_', () => { + // The two prefixes core grandfathers to this module by name + // (loader.js LEGACY_TABLE_PREFIXES). A module written after this one prefixes + // with its own id instead. + const CREATE_TABLE = /CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS\s+`?([A-Za-z0-9_]+)`?/gi + const tables = [] + for (const statement of statements) { + for (const m of statement.matchAll(CREATE_TABLE)) tables.push(m[1].toLowerCase()) + } + assert.ok(tables.length > 20, `expected the module's tables, found ${tables.length}`) + for (const table of tables) { + assert.ok( + table.startsWith('shard_') || table.startsWith('uo_link_'), + `table "${table}" carries neither grandfathered prefix`, + ) + } +}) + +// ── The settings rows this module owns ────────────────────────────────────── + +const SETTINGS_KEYS = ['game_account_signup', 'uo_link_protocol_3_migrated'] + +test('both settings seeds are INSERT IGNORE, so a replay never resets a value', () => { + for (const key of SETTINGS_KEYS) { + const seed = statements.find((s) => /^INSERT/i.test(s) && s.includes(`'${key}'`)) + assert.ok(seed, `no seed for "${key}"`) + assert.match(seed, /^INSERT\s+IGNORE\s+INTO\s+settings/i, key) + } +}) + +test('the protocol-3 marker is written AFTER the update that reads it', () => { + const update = statements.findIndex( + (s) => /^UPDATE\s+uo_link_config/i.test(s) && s.includes('uo_link_protocol_3_migrated'), + ) + const marker = statements.findIndex( + (s) => /^INSERT/i.test(s) && s.includes("'uo_link_protocol_3_migrated'"), + ) + assert.ok(update >= 0, 'the protocol-3 migration is gone') + assert.ok(marker >= 0, 'the one-shot marker is gone') + assert.ok( + marker > update, + 'the marker is written before the UPDATE reads it — the one-shot can never fire. ' + + 'This is the shape of the defect slice 1 introduced by leaving the marker in core, ' + + 'whose schema replays first.', + ) +}) + +test('the migration is guarded on the marker, not on the column value alone', () => { + const update = statements.find((s) => /^UPDATE\s+uo_link_config/i.test(s)) + assert.match(update, /NOT\s+EXISTS\s*\(\s*SELECT/i) + // Without the guard an operator who deliberately pins an older sidecar in + // Admin → Shard is silently re-bumped on the next restart. + assert.match(update, /uo_link_protocol_3_migrated/) +}) -- 2.49.1 From d70e5e10d0d21a969245495b9ba6994041423957 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Tue, 11 Aug 2026 21:31:49 -0500 Subject: [PATCH 2/2] test(client): assert the URLs this module calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five assertions that stayed behind in core's `apiClient.test.js` when the bindings moved in slice 1 — the atlas-vs-shard path split, the query-string filtering, the slug encoding, the admin atlas methods — plus a new one pinning the seven admin URLs the shipped Android app calls by name. They were asserting UO URLs from inside core's suite, which is the boundary Phase 3 removes, and core's slice-4 deletion of those bindings would otherwise have deleted the coverage with them. The fake `window.__rg` carries the REAL react/react-dom/router rather than stubs: `src/core.js` compares its imported bindings against the published ones and logs a "bundled its own copy" error when they differ, so stubs make every run of this file print the exact wording of a real defect. Co-Authored-By: Claude --- client/test/api.test.js | 139 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 client/test/api.test.js diff --git a/client/test/api.test.js b/client/test/api.test.js new file mode 100644 index 0000000..fd843df --- /dev/null +++ b/client/test/api.test.js @@ -0,0 +1,139 @@ +// ── The URLs this module calls ───────────────────────────────────────────── +// +// `src/api.js` binds the paths whose routes live in `server/router/**`, and the +// interesting assertions about it are the ones that encode a DECISION rather +// than a spelling. Three of these came across from core's `apiClient.test.js` +// in slice 4: they had stayed behind when the bindings moved, still asserting +// UO URLs from inside core's suite, which is the boundary this phase removes. +// +// What is NOT re-tested here is the fetch wrapper itself — status mapping, empty +// bodies, FormData, cookie inclusion. That is `req`, core's primitive, and core +// tests it. A module asserting core's contract back at it is a second copy that +// drifts. +// +// The chunk reads its shared bindings off `window.__rg` at module scope +// (src/core.js), so the fake global has to be in place before `src/api.js` is +// imported — hence the dynamic import below rather than a static one. + +import { test, beforeEach, afterEach } from 'node:test' +import assert from 'node:assert/strict' + +import * as react from 'react' +import * as reactDom from 'react-dom/client' +import * as router from 'react-router-dom' +import * as jsxRuntime from 'react/jsx-runtime' + +const BASE = '/api/v1' + +let calls = [] + +function reply({ status = 200, statusText = 'OK', body = '' } = {}) { + return { + ok: status >= 200 && status < 300, + status, + statusText, + text: async () => (typeof body === 'string' ? body : JSON.stringify(body)), + } +} + +// Core's `req`, close enough for a path assertion: the only property this file +// cares about is the URL it was handed. Recording it here rather than mocking +// global.fetch keeps the test honest about the boundary — a module never sees +// fetch, it sees the primitive. +function request(path, opts = {}) { + calls.push({ url: BASE + path, opts }) + return Promise.resolve(reply({ body: {} }).text().then(() => ({}))) +} + +// The REAL react/react-dom/router go in, not stubs: `src/core.js` compares the +// bindings it imported against the ones here and logs a "bundled its own copy" +// error when they differ. With stubs that error fires on every run of this file +// — a false alarm in the exact words of a real defect, which is how a check +// gets ignored. +globalThis.window = globalThis.window || {} +globalThis.window.__rg = { + react, reactDom, router, jsxRuntime, + api: { request, BASE }, + ui: {}, + registry: { registerRoutes() {}, registerNav() {}, registerFeatureProvider() {}, registerExtension() {} }, +} + +const { shard, atlas, admin } = await import('../src/api.js') + +beforeEach(() => { + calls = [] +}) +afterEach(() => { + calls = [] +}) + +// ── spawn atlas (Protocol 3.0 Part C) ─────────────────────────────────────── +// The atlas lives at /public/atlas, NOT under /public/shard: it is static shard +// content parsed from the shard's own files, so it must not look sidecar-backed. +// Asserted because the split is a design decision, not an accident of spelling. +test('atlas reads hit /public/atlas, not /public/shard', async () => { + await atlas.creatures() + assert.equal(calls[0].url, '/api/v1/public/atlas/creatures') +}) + +test('atlas.creatures() sends only the filters that are set', async () => { + await atlas.creatures({ q: 'lizard man', facet: 'Ter Mur', limit: 25 }) + const url = new URL(calls[0].url, 'http://x') + assert.equal(url.pathname, '/api/v1/public/atlas/creatures') + assert.equal(url.searchParams.get('q'), 'lizard man') + assert.equal(url.searchParams.get('facet'), 'Ter Mur') + assert.equal(url.searchParams.get('limit'), '25') + assert.equal(url.searchParams.get('offset'), null) // 0 is not sent +}) + +test('atlas.creature() encodes the slug and carries the facet filter through', async () => { + await atlas.creature('lizardman/rare', { facet: 'Felucca' }) + assert.match(calls[0].url, /\/public\/atlas\/creatures\/lizardman%2Frare\?facet=Felucca$/) +}) + +test('admin atlas actions use the right methods and bodies', async () => { + await admin.atlas.import(true) + assert.equal(calls[0].url, '/api/v1/admin/shard/atlas/import') + assert.equal(calls[0].opts.method, 'POST') + assert.deepEqual(calls[0].opts.body, { force: true }) + + await admin.atlas.setPath('/srv/servuo') + assert.equal(calls[1].opts.method, 'PUT') + assert.deepEqual(calls[1].opts.body, { path: '/srv/servuo' }) +}) + +// ── path encoding ─────────────────────────────────────────────────────────── +// A city name with an apostrophe and a space is the real case: "Serpent's Hold" +// is a governor city, and an unencoded one would break the route match rather +// than 404 cleanly. +test('path params are URL-encoded', async () => { + await shard.governorHistory('Serpent’s Hold', 5) + assert.match(calls[0].url, /\/governors\/Serpent%E2%80%99s%20Hold\/history\?limit=5/) +}) + +// ── the API surface §1.2 freezes ──────────────────────────────────────────── +// The shipped Android app calls these seven by name (data/api/AdminApi.kt), which +// is why the extraction moved which repo declares them and not what they are. A +// rename here is a client break, not a refactor. +test('the seven admin URLs the Android app calls are unchanged', async () => { + const expected = [ + ['kick', '/api/v1/admin/shard/kick'], + ['ban', '/api/v1/admin/shard/ban'], + ['unban', '/api/v1/admin/shard/unban'], + ['broadcast', '/api/v1/admin/shard/broadcast'], + ] + for (const [fn, url] of expected) { + calls = [] + await admin.shardOps[fn]({}) + assert.equal(calls[0].url, url, fn) + } + calls = [] + await admin.shardOps.pages() + assert.equal(calls[0].url, '/api/v1/admin/shard/pages') + calls = [] + await admin.shardOps.respondPage('7', {}) + assert.equal(calls[0].url, '/api/v1/admin/shard/pages/7/respond') + calls = [] + await admin.shardOps.closePage('7') + assert.equal(calls[0].url, '/api/v1/admin/shard/pages/7/close') +}) -- 2.49.1