Merge pull request 'fix(atlas): keep the UniqueId, and make a landmark value name one landmark' (#35) from fix/atlas-unique-id-and-landmark-values into main

Reviewed-on: #35

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

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) {
@@ -481,12 +519,29 @@ async function placeOwned({ runId, idempotencyKey, what, body }) {
  * shard denies this run ever owned that serial — nothing will ever delete it
  * through this path, so the row must land unresolved with a reason rather than
  * be quietly marked reverted.
+ *
+ * **The despawn carries NO idempotency key, and that is the whole point.** This
+ * function used to forward core's `idempotencyKey` as the despawn's own — which
+ * is the step's key, the very key `placeOwned` spawned under. The shard's
+ * at-most-once store is keyed on the key ALONE (`BridgeIdempotency.Intercept`
+ * does `_byKey.TryGetValue(key, …)`, not a lookup by key AND command), so the
+ * despawn was recognised as a repeat and answered with the SPAWN's stored reply.
+ * `OnDespawn` never ran, core saw `ok` with no `refused`, and every row was
+ * marked `reverted` while the shard still held every object. Teardown of all
+ * five world verbs was a no-op that reported success.
+ *
+ * No key is needed here. A repeat despawn is already safe by the handler's own
+ * three-answer design: the second pass finds the serial gone and answers `gone`,
+ * which is a success on both ends. `MODULE_API.md` says what core's key is FOR,
+ * and it is not this — it identifies a dispatch core never learned the outcome
+ * of, so the module can ask about it. That case arrives here as an EMPTY
+ * `resources` list, and it is answered correctly by the same call: no serials
+ * means "everything this run owns", which is exactly the right sweep.
  */
-async function revertOwned({ runId, resources, idempotencyKey }) {
+async function revertOwned({ runId, resources }) {
   const result = await uoLinkClient.despawnWorld({
     runId: String(runId),
     serials: resources.map((resource) => resource.ref),
-    idempotencyKey,
   })
   if (!result.ok) return { ok: false, error: sidecarReason(result, 'despawn') }
 
@@ -2108,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

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`),
       `<Spawns>
-        <Points><Name>${facet}A</Name><Map>${facet}</Map><X>1100</X><Y>1100</Y>
+        <Points><Name>${facet}A</Name><UniqueId>uid-${facet}-A</UniqueId>
+          <Map>${facet}</Map><X>1100</X><Y>1100</Y>
           <MaxCount>3</MaxCount><IsRunning>True</IsRunning>
           <Objects2>Lizardman:MX=3:SB=0:OBJ=Orc:MX=1:SB=0</Objects2></Points>
         <Points><Name>${facet}B</Name><Map>${facet}</Map><X>9000</X><Y>9000</Y>
@@ -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 <UniqueId> 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)
@@ -440,6 +464,42 @@ test('decoration is read recursively and rolled up per type', () => {
   }
 })
 
+test('two spellings of one decoration type fold into one row', () => {
+  // The Phase 16 acceptance walk's blocking finding. Stock ServUO 57.4's own
+  // `Data/Decoration/` names four types under two casings each —
+  // CheckerBoard/Checkerboard, ChessBoard/Chessboard, MetalChest/Metalchest,
+  // SpinningWheelEastAddon/SpinningwheelEastAddon — and in every pair exactly one
+  // is a real class; the other is a mis-cased line the shard's own loader resolves
+  // anyway.
+  //
+  // A case-SENSITIVE Map keeps both. `shard_decor_types.type` is a PRIMARY KEY
+  // under MariaDB's default `..._ai_ci` collation, which folds case, so the second
+  // row raised `1062 Duplicate entry` and took the WHOLE atlas import transaction
+  // down with it. The blast radius is not decoration: with no atlas, EVERY option
+  // source answers empty and no world verb can be authored at all.
+  //
+  // Asserted on the count as well as the row, because the failure mode was two
+  // rows that a database — not this function — would later refuse.
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), 'atlas-decorcase-'))
+  try {
+    writeTree(root)
+    fs.writeFileSync(
+      path.join(root, 'Data', 'Decoration', 'miscased.cfg'),
+      'checkerboard 0x0FA6\n600 600 0\nCheckerBoard 0x0FA6\n700 700 0\n',
+    )
+    const atlas = buildAtlas(root)
+
+    const boards = atlas.decor.filter((d) => d.type.toLowerCase() === 'checkerboard')
+    assert.equal(boards.length, 1, 'two casings of one type must not be two rows')
+    // First spelling seen wins, exactly as the first item id does. Which one
+    // survives is cosmetic — the shard resolves either.
+    assert.equal(boards[0].type, 'checkerboard')
+    assert.equal(boards[0].uses, 2, 'both lines still count as uses of the one type')
+  } finally {
+    fs.rmSync(root, { recursive: true, force: true })
+  }
+})
+
 test('a tree with no decoration at all still builds', () => {
   // Optional, like the champion file. A shard that has stripped its decoration
   // has a perfectly good atlas; the decoration verb simply has nothing to offer.

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 () => {
@@ -766,6 +838,51 @@ test('teardown reports a refused serial as failed, and a killed creature as done
   assert.equal((await actions.revertOwned({ runId: 7, resources })).ok, false)
 })
 
+test('the despawn carries NO idempotency key, whatever core hands revert()', async () => {
+  // The Phase 16 acceptance walk's critical finding, as the test that would have
+  // caught it. `revertOwned` used to forward core's `idempotencyKey` onto the
+  // despawn — and core's key is the STEP's, the one `placeOwned` spawned under.
+  // The shard's at-most-once store is keyed on the key ALONE
+  // (`BridgeIdempotency.Intercept` does `_byKey.TryGetValue(key, …)`, with no
+  // reference to which command carried it), so the despawn was taken for a repeat
+  // and answered with the SPAWN's stored reply. `OnDespawn` never ran. Core read
+  // `ok` with no `refused` and marked every row `reverted` while the shard still
+  // held every object — teardown of all five world verbs was a no-op that
+  // reported success.
+  //
+  // Every other stub in this file ignores the body, which is why the suite was
+  // green throughout. This one asserts on the body, and it asserts ABSENCE — the
+  // property that matters — rather than pinning the rest of the shape.
+  let sent = null
+  uoLinkClient.despawnWorld = async (body) => {
+    sent = body
+    return { ok: true, status: 200, data: { removed: ['0x40000000'], gone: [], refused: [] } }
+  }
+
+  await actions.revertOwned({
+    runId: 7,
+    resources: [{ kind: 'world', ref: '0x40000000', payload: {} }],
+    // Core passes this on every call (MODULE_API.md), and it must not reach the wire.
+    idempotencyKey: 'the-step-key-the-spawn-went-out-under',
+  })
+
+  assert.ok(sent, 'despawnWorld was not called')
+  assert.equal(
+    Object.prototype.hasOwnProperty.call(sent, 'idempotencyKey'),
+    false,
+    'the despawn must not carry an idempotency key — the shard would replay the spawn',
+  )
+
+  // MODULE_API.md: revert is sometimes called with the key and an EMPTY list,
+  // meaning "a command went out under this key and core never learned what it
+  // did". No serials is the shard's own idiom for "everything this run owns",
+  // which is the correct sweep for exactly that case.
+  sent = null
+  await actions.revertOwned({ runId: 7, resources: [], idempotencyKey: 'lost-dispatch' })
+  assert.deepEqual(sent.serials, [])
+  assert.equal(Object.prototype.hasOwnProperty.call(sent, 'idempotencyKey'), false)
+})
+
 test('reconcile ASKS the shard, because these resources survive a restart', async () => {
   // The one property that separates this from every other resource in the file.
   // A crier line lives in shard memory, so a changed `bootId` IS proof it is

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,
@@ -336,11 +351,29 @@ function buildAtlas(root, options = {}) {
 
   // Decoration: what this shard already calls scenery, which is what makes the
   // authoring dropdown the operator's own vocabulary rather than our taste.
+  //
+  // **Keyed case-INSENSITIVELY, because the decoration files disagree with
+  // themselves about casing.** Stock 57.4 names four types under two spellings
+  // each — `CheckerBoard`/`Checkerboard`, `ChessBoard`/`Chessboard`,
+  // `MetalChest`/`Metalchest`, `SpinningWheelEastAddon`/`SpinningwheelEastAddon`
+  // — and in every pair exactly one is a real class, the other a mis-cased line
+  // the shard's own loader resolves anyway. A case-sensitive Map keeps both, and
+  // then `shard_decor_types.type` (a PRIMARY KEY under MariaDB's default
+  // `..._ai_ci` collation, which folds case) rejects the second row and takes the
+  // WHOLE import transaction down with it. That is not a decoration bug: with no
+  // atlas, every option source answers empty and no world verb can be authored at
+  // all. The shard end of this feature already knew — `BridgeWorld.cs` resolves a
+  // decor type with `FindTypeByName(name, ignoreCase: true)` and says why — so
+  // folding here is the two ends agreeing rather than a new rule.
+  //
+  // The first spelling seen wins, exactly as the first item id does. Either
+  // spelling resolves on the shard, so which one survives is cosmetic.
   const decorUses = new Map()
   for (const file of files) {
     if (!file.label.startsWith('Data/Decoration/')) continue
     for (const entry of parseDecoration(file.text)) {
-      const seen = decorUses.get(entry.type)
+      const key = entry.type.toLowerCase()
+      const seen = decorUses.get(key)
       if (seen) {
         seen.uses += 1
         continue
@@ -348,7 +381,7 @@ function buildAtlas(root, options = {}) {
       // The FIRST item id wins, and it is only a preview: a type appears under
       // as many ids as it has facings or variants, and picking one arbitrarily
       // is honest in a way that picking "the most used" would not be.
-      decorUses.set(entry.type, { type: entry.type, itemId: entry.itemId, uses: 1 })
+      decorUses.set(key, { type: entry.type, itemId: entry.itemId, uses: 1 })
     }
   }
   const decor = [...decorUses.values()].sort((a, b) => a.type.localeCompare(b.type))

server/utils/uoLinkClient.js

@@ -330,10 +330,18 @@ const ownedWorld = ({ runId }) => call(`/world/${encodeURIComponent(runId)}`)
 // teardown makes. The reply splits three ways: `removed` was deleted, `gone` was
 // already absent (a player killed it — an ordinary success), and `refused` was never
 // this run's to delete.
-const despawnWorld = ({ runId, serials, idempotencyKey }) =>
+//
+// **It takes no idempotency key, and the parameter is gone rather than optional.**
+// It used to accept one, and `revertOwned` passed the step's — the key the SPAWN
+// went out under. The shard's at-most-once store is keyed on the key alone, so the
+// despawn was answered with the spawn's stored reply and nothing was ever deleted.
+// A repeat despawn needs no key: the second pass answers `gone`, which both ends
+// already treat as a success. Removed from the signature so it cannot be handed
+// one again by accident.
+const despawnWorld = ({ runId, serials }) =>
   call(`/world/${encodeURIComponent(runId)}/despawn`, {
     method: 'POST',
-    body: { serials, idempotencyKey },
+    body: { serials },
   })
 
 // ── Help-page (support) queue commands (§6) ────────────────────────────────