Merge pull request 'fix(db): own the two settings seeds, and repair the protocol-3 one-shot (phase 3, slice 4)' (#5) from feature/own-settings-seeds into main

Reviewed-on: #5

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

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`;

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

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