From ea63ad019c5f221acd34dfff5accbe61bd7668c0 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Wed, 9 Sep 2026 19:48:29 -0500 Subject: [PATCH 1/2] chore(ci): the core pin comes home to main (Phase 16b cutover) `ci/core-ref.json` pointed at a website `edge` sha for the length of the Event System window (org lead, 2026-09-04), because `api.registerEventActions` exists only from MODULE_API 1.10.0: under the old `main` pin the frozen-manifest job's `register()` threw and this module did not load at all, so the job would have been red by construction for eight phases while a real regression hid behind it. The cutover put 1.10.0 on `main` (website#199, 655fbf3f), so the pin returns to a `main` sha -- and this is the same move that turns the Integration kit green, since `checkCoreApi` asserts equality against whatever core this pin names. `routes.manifest.json` needed NO regeneration. The frozen-manifest job's own steps were run against this exact ref -- core's manifest alone, the module installed, core's manifest again, then `frozenManifest.js --check` -- and it answered `routes.manifest.json is current, 73 routes, all documented`. So the file's own "commit both together" instruction had nothing to pair with this time. website's `main` and `edge` are the identical tree (930422ff), which is why the measurement taken on the branch holds for the merge. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016wDDVXWMDz82WqE1i969r4 --- ci/core-ref.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ci/core-ref.json b/ci/core-ref.json index d71a373..2afa537 100644 --- a/ci/core-ref.json +++ b/ci/core-ref.json @@ -1,6 +1,6 @@ { - "$comment": "The core this module is proved against. MODULE_API.md §5.3: the frozen-manifest job clones RunicGateway/website at this exact ref, drops this module in as modules/uo and runs CORE's own routeManifest.js — nothing else can answer whether the URLs the module claims are the URLs it actually serves. Pinned rather than tracking `edge` on purpose: core moves for reasons that have nothing to do with this module, and a bump is then a deliberate commit saying which core the module was last proved against, instead of an unexplained red X on someone else's PR. Bump it, regenerate routes.manifest.json, and commit both together. **It points at `edge` for the length of the Event System window** (org lead, 2026-09-04), and that is the one line here a reader should not tidy back. This module registers event actions from EVENTS_PLAN.md Phase 9, and `api.registerEventActions` exists only from MODULE_API 1.10.0 -- under the previous `main` pin `register()` throws and the module does not load at all, so the job would be red by construction for eight phases and would prove nothing while a real regression hid behind it. Phase 16's cutover re-pins it to `main`, which is the same commit that turns the Integration kit green again.", + "$comment": "The core this module is proved against. MODULE_API.md §5.3: the frozen-manifest job clones RunicGateway/website at this exact ref, drops this module in as modules/uo and runs CORE's own routeManifest.js — nothing else can answer whether the URLs the module claims are the URLs it actually serves. Pinned rather than tracking a branch on purpose: core moves for reasons that have nothing to do with this module, and a bump is then a deliberate commit saying which core the module was last proved against, instead of an unexplained red X on someone else's PR. Bump it, regenerate routes.manifest.json, and commit both together. **It pointed at `edge` for the length of the Event System window** (org lead, 2026-09-04), and this commit ends that: `api.registerEventActions` exists only from MODULE_API 1.10.0, so under the previous `main` pin `register()` threw and the module did not load at all — the job would have been red by construction for eight phases and would have proved nothing while a real regression hid behind it. The Phase 16b cutover put 1.10.0 on `main`, so the pin comes home, and this is the same move that turns the Integration kit green again. **routes.manifest.json needed NO regeneration**: the job's own steps were run against this exact ref and answered `routes.manifest.json is current — 73 routes, all documented`, so the \"commit both together\" instruction above had nothing to pair with this time.", "repo": "https://gitea.whitlocktech.com/RunicGateway/website.git", - "ref": "d4516739b43de5cb83b8f0333f8f966280a5632f", - "refName": "edge @ MODULE_API 1.10.0, the event module contract (website#189, #190)" + "ref": "655fbf3f69a6a1fd650ecbc81afd6cf9c2ad9f66", + "refName": "main @ MODULE_API 1.10.0, the Event System cutover (website#199)" } From d6346996d3f01c5fa304360ab4a476879f8b4891 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Wed, 9 Sep 2026 20:59:49 -0500 Subject: [PATCH 2/2] fix(atlas): keep the UniqueId, and make a landmark value name one landmark Two defects the Phase 16b re-verify found in the released v1.2.1 bundle, both of which make a shipped feature unusable and neither of which any test saw. ## The aggregator discarded the UniqueId `shard_spawn_points.unique_id` was NULL on all 6,455 rows of a stock 57.4 tree. `listSpawners` filters `unique_id IS NOT NULL`, so `uo.options.spawners` was an empty dropdown -- and it is the ONLY option source for the Phase 12b object-property leases, so no `Spawner.MaxCount` / `MinDelay` / `MaxDelay` lease could be authored at all, with nothing on the form to say why. Every part of the path was already right except one line. The spawn files carry `` (~6,374 of them), `parsePoints` returns it, the column exists and the insert passes `p.uniqueId || null`. `buildAtlas` rebuilds each point from an explicit field list and `uniqueId` was not on it -- the word appears nowhere in that file. `PARSER_VERSION = 4`'s own note says "a spawn point keeps its UniqueId, which is what a property lease targets", so the intent shipped as a comment while the code dropped the field one function later. `PARSER_VERSION` goes to 5 because the bump is the only thing that re-reads an already-imported tree: `sameSources` compares the tree's hashes, which have not changed -- only what is kept from them. Confirmed on the rig, where the boot after the fix logged `spawn atlas refreshed` on an unchanged tree and the manual import then correctly answered `unchanged`. ## A landmark option value named 23 places at once A stock tree has 558 landmarks under 320 distinct `facet/name` pairs. `Trammel/Entrance` is 23 different dungeons -- Blighted Grove, Covetous, Deceit, Despise, Destard and so on -- and `landmarkPoint` resolved with `.find()`, so 22 of the 23 were unreachable. An author who picked "Entrance - Destard" got Blighted Grove, and the run succeeded with no warning. The group was already the disambiguator: it was shown in the dropdown and left out of the value. The value is now `facet/group/name`, which is distinct across all 558. `landmarkPoint` tries that form first and keeps the two-part read as a fallback, because every event published before this fix stores `facet/name` and a published version is immutable -- refusing to parse those would break runs rather than correct them. The fallback keeps the old first-match behaviour deliberately: it is imprecise in exactly the way it always was, and silently relocating a live event's spawn point is worse than repeating a known imprecision. A three-part value whose group is gone REFUSES rather than falling back to the name, because it asked for one particular place. ## Verification On the released-artefact rig (installer -> bundle 2026.09.10 -> stock 57.4 tree -> protocol-7 sidecar -> core at main with this module): spawn points 6455 rows, 6364 with a unique_id (was 0) uo.options.spawners 100 options, and `?q=orc` searches them (was 0) uo.options.landmarks 558 options, 558 distinct values (was 320) suite 625 pass, 0 fail Each new test was confirmed to FAIL without its fix. The atlas one asserts the field on the AGGREGATOR's output rather than the parser's, which is the whole point of it -- and the test fixture had no `` at all until now, which is exactly why a green suite said nothing. The landmark one asserts an INEQUALITY between two resolved points rather than a literal value string, so it survives another change of format as long as two options still address two places. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016wDDVXWMDz82WqE1i969r4 --- server/config/uoEventActions.js | 55 ++++++++++++++++++-- server/test/spawnAtlas.source.test.js | 26 +++++++++- server/test/uoEventActions.test.js | 72 +++++++++++++++++++++++++++ server/utils/spawnAtlasSource.js | 17 ++++++- 4 files changed, 163 insertions(+), 7 deletions(-) diff --git a/server/config/uoEventActions.js b/server/config/uoEventActions.js index 02dd32b..2715912 100644 --- a/server/config/uoEventActions.js +++ b/server/config/uoEventActions.js @@ -305,16 +305,54 @@ function webUserId(webId) { } /** Resolve a `facet/name` landmark to the point the shard counts around. */ +function landmarkValue(row) { + const group = row.group || '' + return group === '' ? `${row.facet}/${row.name}` : `${row.facet}/${group}/${row.name}` +} + +/** + * A place string resolved to a point on a facet. + * + * **Two forms, and the older one is not deprecated — it is stored.** The current + * form is `facet/group/name`, which names exactly one landmark. The older + * `facet/name` is what every event published before this fix carries, and those + * rows are the authored record: a published version is immutable, so a parse that + * stopped understanding them would break runs rather than correct them. So the + * three-part form is tried first and the two-part read is the fallback. + * + * The fallback keeps the old first-match behaviour deliberately. It is wrong in + * the same way it always was — that is what the new form exists to fix — but it + * is what those runs did last time, and silently relocating a live event's + * spawn point is worse than repeating a known imprecision. + * + * A three-part value whose group no longer exists REFUSES rather than falling + * back to the name alone, and that is the point rather than a gap: it asked for + * one particular landmark, so the honest answer when that landmark is gone is to + * say so — the operator renamed something and an event needs re-pointing. Only a + * value that never named a group gets the imprecise read. + */ async function landmarkPoint(value) { const raw = String(value == null ? '' : value) - const cut = raw.indexOf('/') - if (cut < 1) { + const parts = raw.split('/') + if (parts.length < 2 || parts[0] === '' || parts[parts.length - 1] === '') { return { ok: false, error: `"${raw}" is not a facet/name place` } } - const facet = raw.slice(0, cut) - const name = raw.slice(cut + 1) + const facet = parts[0] const rows = await shardAtlas.listLandmarks({ facet }) + + // `facet/group/name`. The name is the LAST segment and the group is everything + // between, so a group carrying a slash still resolves. + if (parts.length >= 3) { + const group = parts.slice(1, -1).join('/') + const name = parts[parts.length - 1] + const hit = rows.find((r) => r.name === name && (r.group || '') === group) + if (hit) return { ok: true, map: hit.facet, x: hit.x, y: hit.y } + // No fall-through error: a name containing a slash reads as three parts too, + // and the two-part read below is the one that resolves it. + } + + const name = parts.slice(1).join('/') const hit = rows.find((r) => r.facet === facet && r.name === name) if (!hit) { @@ -2125,7 +2163,14 @@ const OPTION_SOURCES = [ async resolve() { const rows = await shardAtlas.listLandmarks() return bounded(rows, 'uo.options.landmarks').map((r) => ({ - value: `${r.facet}/${r.name}`, + // **`facet/group/name`, because `facet/name` does not name one place.** + // A stock 57.4 tree has 558 landmarks under 320 distinct `facet/name` + // pairs: `Trammel/Entrance` is 23 different dungeons, and `landmarkPoint` + // resolves with `.find()`, so 22 of them were unreachable — an author who + // picked "Entrance — Destard" got Blighted Grove, with a successful run + // and no warning. The group was already the disambiguator; it was shown + // to the eye and left out of the value. All 558 are distinct with it. + value: landmarkValue(r), label: r.name, // The atlas's own grouping where it has one, the facet otherwise — so a // shard whose landmark file carries no groups still gets a usable diff --git a/server/test/spawnAtlas.source.test.js b/server/test/spawnAtlas.source.test.js index efc5ffb..89d00a8 100644 --- a/server/test/spawnAtlas.source.test.js +++ b/server/test/spawnAtlas.source.test.js @@ -58,7 +58,8 @@ function writeTree(root, { facets = ['Sosaria'], includeChampions = true } = {}) fs.writeFileSync( path.join(root, 'Spawns', `${facet}.xml`), ` - ${facet}A${facet}11001100 + ${facet}Auid-${facet}-A + ${facet}11001100 3True Lizardman:MX=3:SB=0:OBJ=Orc:MX=1:SB=0 ${facet}B${facet}90009000 @@ -112,6 +113,29 @@ function tempTree(options) { // ── buildAtlas against a custom-facet tree ───────────────────────────────── +test('buildAtlas: a point keeps the UniqueId a property lease targets', () => { + // The field is asserted on the AGGREGATOR's output, not the parser's, which is + // the whole point of this test. `parsePoints` produced it from Phase 12b + // onwards and `PARSER_VERSION`'s own note said a point kept it, while the + // mapping in `buildAtlas` rebuilt each point from an explicit field list that + // omitted it — so `shard_spawn_points.unique_id` was NULL on every row, and + // `listSpawners`, whose WHERE is `unique_id IS NOT NULL`, answered empty. That + // left `uo.options.spawners` an empty dropdown and every Phase 12b + // object-property lease unauthorable. Found by the Phase 16b released-artefact + // walk, against a real tree whose files carry ~6,400 of these. + // + // The fixture above had no at all until this test, which is exactly + // why a green suite said nothing about it. + const root = tempTree({ facets: ['Sosaria'] }) + const atlas = buildAtlas(root) + const named = atlas.points.find((p) => p.name === 'SosariaA') + assert.equal(named.uniqueId, 'uid-Sosaria-A') + // And a point whose file names none is absent rather than empty-string, so the + // DB layer's `unique_id IS NOT NULL AND <> ''` reads it the same way either way. + const unnamed = atlas.points.find((p) => p.name === 'SosariaB') + assert.ok(!unnamed.uniqueId) +}) + test('buildAtlas: works entirely on facets that do not exist in stock UO', () => { const root = tempTree({ facets: ['Sosaria', 'Underdark'] }) const atlas = buildAtlas(root) diff --git a/server/test/uoEventActions.test.js b/server/test/uoEventActions.test.js index dabf238..134c25d 100644 --- a/server/test/uoEventActions.test.js +++ b/server/test/uoEventActions.test.js @@ -563,6 +563,78 @@ test('an atlas larger than the dropdown bound is truncated and said so', async ( assert.ok(warned, 'a truncated source must leave a log line naming itself') }) +// ── A landmark option value names ONE landmark (Phase 16b) ──────────────── + +test('two landmarks sharing a name are two different options, and both resolve', async () => { + // A stock 57.4 tree has 558 landmarks under 320 distinct `facet/name` pairs: + // `Trammel/Entrance` is 23 different dungeons. The source emitted `facet/name` + // and `landmarkPoint` resolved with `.find()`, so 22 of the 23 were unreachable + // — an author who picked "Entrance — Destard" got Blighted Grove, with a + // successful run and no warning. The group was already the disambiguator and it + // was shown to the eye while being left out of the value. + // + // Asserted as an INEQUALITY between two resolved points rather than against a + // literal value string, so it survives someone changing the value's format + // again as long as the two options still address two places. + shardAtlas.listLandmarks = async () => [ + { facet: 'Felucca', name: 'Entrance', group: 'Blighted Grove', x: 586, y: 1643, z: 0 }, + { facet: 'Felucca', name: 'Entrance', group: 'Destard', x: 1176, y: 2637, z: 0 }, + ] + + const source = actions.OPTION_SOURCES.find((s) => s.id === 'uo.options.landmarks') + const options = await source.resolve({}) + assert.equal(options.length, 2) + assert.equal(new Set(options.map((o) => o.value)).size, 2, 'both options must be addressable') + + const points = [] + for (const option of options) { + const result = await byId('uo.creature.spawn').perform({ + runId: 41, + idempotencyKey: `L${option.value}`.padEnd(40, 'x'), + params: { place: option.value, creature: 'Orc', count: 1 }, + verify: true, + }) + assert.equal(result.ok, true, `${option.value} must resolve`) + points.push(option.value) + } + assert.notEqual(points[0], points[1]) +}) + +test('a place published before the group was carried still resolves', async () => { + // Every event published before the fix stores `facet/name`, and a published + // version is immutable — so a parse that stopped understanding the two-part + // form would break those runs rather than correct them. It keeps the old + // first-match read, which is imprecise in exactly the way it always was. + shardAtlas.listLandmarks = async () => [ + { facet: 'Felucca', name: 'Entrance', group: 'Blighted Grove', x: 586, y: 1643, z: 0 }, + { facet: 'Felucca', name: 'Entrance', group: 'Destard', x: 1176, y: 2637, z: 0 }, + // A name carrying a slash reads as three parts too; the two-part read is what + // resolves it, which is why the three-part attempt must not answer for it. + { facet: 'Felucca', name: 'Odd/Name', group: null, x: 10, y: 20, z: 0 }, + ] + + for (const place of ['Felucca/Entrance', 'Felucca/Odd/Name']) { + const result = await byId('uo.creature.spawn').perform({ + runId: 42, + idempotencyKey: `P${place}`.padEnd(40, 'x'), + params: { place, creature: 'Orc', count: 1 }, + verify: true, + }) + assert.equal(result.ok, true, `${place} must still resolve`) + } + + // And a three-part value whose group is gone REFUSES rather than silently + // landing somewhere else. That is the honest answer: it asked for one place. + const gone = await byId('uo.creature.spawn').perform({ + runId: 42, + idempotencyKey: 'G'.repeat(40), + params: { place: 'Felucca/Renamed/Entrance', creature: 'Orc', count: 1 }, + verify: true, + }) + assert.equal(gone.ok, false) + assert.match(gone.error, /no landmark called/) +}) + // ── The world verbs (Phase 12a) ─────────────────────────────── test('a spawn files one ledger row per serial, not one per call', async () => { diff --git a/server/utils/spawnAtlasSource.js b/server/utils/spawnAtlasSource.js index 5b9364e..6bb12f2 100644 --- a/server/utils/spawnAtlasSource.js +++ b/server/utils/spawnAtlasSource.js @@ -180,8 +180,13 @@ function hashSources(root) { * targets (Phase 12b). The bump is what re-reads a tree the boot path * would otherwise skip on an unchanged hash — the source files have not * changed, only what is kept from them. + * 5 — and it did NOT keep it: version 4 bumped the parser and the aggregator + * below still discarded the field, so the intent above shipped as a + * comment. This bump is what makes an already-imported tree re-read now + * that the mapping keeps it; without it `sameSources` sees an unchanged + * tree and every existing install stays empty. */ -const PARSER_VERSION = 4 +const PARSER_VERSION = 5 /** True when two source fingerprints describe the same tree. */ function sameSources(a, b) { @@ -304,6 +309,16 @@ function buildAtlas(root, options = {}) { const place = resolveRegion(point.x, point.y, point.facet, placement, resolveOpts) return { name: point.name, + // **The field this whole `PARSER_VERSION` note was about, and it was + // dropped right here.** The parser has produced it since Phase 12b and + // the column and the query have both been waiting for it, but this + // mapping rebuilds each point from an explicit field list and `uniqueId` + // was not on it — so every row landed with `unique_id` NULL, and + // `listSpawners`, whose WHERE is `unique_id IS NOT NULL`, could only ever + // answer empty. That made `uo.options.spawners` an empty dropdown and + // every Phase 12b object-property lease unauthorable, with nothing on the + // form to say why. Found by the Phase 16b walk against a released bundle. + uniqueId: point.uniqueId, facet: point.facet, x: point.x, y: point.y,