Merge pull request 'feat(teams): ingest guild rank, and report every leader rather than one' (#10) from feat/protocol4-guild-rank into edge

Reviewed-on: #10

server/db/schema.sql

@@ -241,11 +241,29 @@ CREATE TABLE IF NOT EXISTS shard_guild_members (
   acct       VARCHAR(120) NULL,                 -- absent for a mobile with no account
   web_id     INT          NULL,                 -- set only when the account is linked
   is_player  TINYINT(1)   NOT NULL DEFAULT 1,
+  -- Guild rank, 0-4, with 4 being Leader (ServUO RankDefinition.Ranks). NULL means
+  -- "not known", which is a real state and not a demotion: the shard omits the rank
+  -- for a staff account, because PlayerMobile.GuildRank reports Leader for anyone at
+  -- GameMaster or above whatever their actual rank, and publishing that would put a
+  -- staff member on a public roster as a guild leader.
+  -- Backticked, like `int` on shard_online: RANK is a reserved word in MySQL 8 and
+  -- a non-reserved keyword in MariaDB, so it parses here bare but must not be
+  -- written that way anywhere it might not.
+  `rank`      TINYINT      NULL,
+  -- The rank's NAME, as the game states it: a cliloc id for the five standard ranks
+  -- (1062959-1062963, which ship with no text), or a literal string when a shard has
+  -- replaced the rank table with custom definitions. Resolving one to a label is this
+  -- module's job -- it owns the cliloc table and the game vocabulary.
+  rank_cliloc INT          NULL,
+  rank_name   VARCHAR(64)  NULL,
   t          BIGINT       NULL,                 -- roster event time, epoch ms
   updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
   PRIMARY KEY (guild_id, serial),
   INDEX idx_shard_guild_members_acct (acct),
-  INDEX idx_shard_guild_members_web (web_id)
+  INDEX idx_shard_guild_members_web (web_id),
+  -- Leadership is "rank >= 4", asked per guild, which is the query the Team provider
+  -- runs on every reconcile.
+  INDEX idx_shard_guild_members_rank (guild_id, rank)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;
 
 -- Town-governor board (Protocol 2.0, City Loyalty). One row per city, upserted on
@@ -670,3 +688,13 @@ INSERT IGNORE INTO settings (`key`, value) VALUES ('uo_link_protocol_3_migrated'
 -- 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');
+-- Protocol 4 guild rank, added to databases that already have shard_guild_members.
+--
+-- The table itself is new in Protocol 4 and unreleased, so no production install has
+-- it — but `edge` deployments do, from the roster work that landed before the rank
+-- amendment, and CREATE TABLE IF NOT EXISTS adds a table and never a column. This is
+-- the same gap the sidecar's own store hit when `guilds.members` was added.
+ALTER TABLE shard_guild_members ADD COLUMN IF NOT EXISTS `rank` TINYINT NULL;
+ALTER TABLE shard_guild_members ADD COLUMN IF NOT EXISTS rank_cliloc INT NULL;
+ALTER TABLE shard_guild_members ADD COLUMN IF NOT EXISTS rank_name VARCHAR(64) NULL;
+ALTER TABLE shard_guild_members ADD INDEX IF NOT EXISTS idx_shard_guild_members_rank (guild_id, `rank`);

server/model/shardState/shardState.db.js

@@ -172,19 +172,32 @@ const clearGuilds = () => query('DELETE FROM shard_guilds')
 const listGuilds = () => query(`SELECT ${GUILD_COLS} FROM shard_guilds ORDER BY name ASC`)
 
 // ── Guild membership (Protocol 4) ──────────────────────────────────────────
-const MEMBER_COLS = 'guild_id, serial, name, acct, web_id, is_player, t'
+// `rank` is backticked wherever it is written, like `int` on shard_online: it is a
+// reserved word in MySQL 8 and merely a keyword in MariaDB, so it parses bare here
+// and must not be relied on to.
+const MEMBER_COLS = 'guild_id, serial, name, acct, web_id, is_player, `rank`, rank_cliloc, rank_name, t'
 
 // Upsert rather than plain insert: a roster frame can be redelivered (the /history
 // backfill replays stored frames on every reconnect), and a redelivery must be a
 // no-op rather than a duplicate-key error.
+//
+// The rank columns are assigned unconditionally, NULL included. A member whose rank
+// the shard withheld — a staff account, whose GuildRank getter reports Leader
+// regardless of the truth — must go back to "not known" rather than keeping a rank
+// from before they were promoted.
 const upsertGuildMembers = (rows) => {
   if (!rows.length) return Promise.resolve()
-  const values = rows.map(() => '(?, ?, ?, ?, ?, ?, ?)').join(', ')
+  const values = rows.map(() => '(?, ?, ?, ?, ?, ?, ?, ?, ?, ?)').join(', ')
-  const params = rows.flatMap((r) => [r.guild_id, r.serial, r.name, r.acct, r.web_id, r.is_player, r.t])
+  const params = rows.flatMap((r) => [
+    r.guild_id, r.serial, r.name, r.acct, r.web_id, r.is_player,
+    r.rank, r.rank_cliloc, r.rank_name, r.t,
+  ])
   return query(
     `INSERT INTO shard_guild_members (${MEMBER_COLS}) VALUES ${values}
      ON DUPLICATE KEY UPDATE name = VALUES(name), acct = VALUES(acct),
-       web_id = VALUES(web_id), is_player = VALUES(is_player), t = VALUES(t)`,
+       web_id = VALUES(web_id), is_player = VALUES(is_player),
+       \`rank\` = VALUES(\`rank\`), rank_cliloc = VALUES(rank_cliloc),
+       rank_name = VALUES(rank_name), t = VALUES(t)`,
     params,
   )
 }

server/model/shardState/shardState.model.js

@@ -386,6 +386,14 @@ async function upsertGuildRoster(ev) {
       acct: m.acct ?? null,
       web_id: Number.isFinite(m.webId) ? m.webId : null,
       is_player: m.player ? 1 : 0,
+      // Guild rank (Protocol 4). ABSENT is a real state and is stored as NULL: the
+      // shard withholds the rank for a staff account, because ServUO's GuildRank
+      // getter reports Leader for anyone at GameMaster or above whatever their
+      // actual rank. Defaulting a missing rank to 0 here would turn "we were not
+      // told" into "rank 0", which is a demotion invented by this line.
+      rank: Number.isInteger(m.rank) ? m.rank : null,
+      rank_cliloc: Number.isInteger(m.rankCliloc) ? m.rankCliloc : null,
+      rank_name: typeof m.rankName === 'string' && m.rankName ? m.rankName.slice(0, 64) : null,
       t: Number.isFinite(ev.t) ? ev.t : null,
     }))
 

server/model/teamProvider/teamProvider.db.js

@@ -10,6 +10,9 @@
 // though `ctx` does not exist yet when this file is first required.
 const { query } = require('../../core')
 
+/** ServUO's `RankDefinition.Ranks[4]` is Leader, and 4 is the top of the ladder. */
+const LEADER_RANK = 4
+
 /**
  * The guild board — one row per guild the shard has told us about.
  *
@@ -53,8 +56,8 @@ const findGuild = (id) =>
  */
 const listGuildMembers = (guildId) =>
   query(
-    `SELECT m.serial, m.name, m.acct, m.web_id, m.is_player,
+    "SELECT m.serial, m.name, m.acct, m.web_id, m.is_player, m.`rank`, m.rank_cliloc, m.rank_name, " +
-            l.user_id AS linked_user_id,
+    `        l.user_id AS linked_user_id,
             (o.serial IS NOT NULL) AS is_online
      FROM shard_guild_members m
      LEFT JOIN shard_account_links l ON l.account = m.acct
@@ -64,4 +67,21 @@ const listGuildMembers = (guildId) =>
     [guildId],
   )
 
-module.exports = { listGuilds, findGuild, listGuildMembers }
+/**
+ * Every member at leader rank — rank 4, the top of ServUO's `RankDefinition.Ranks`.
+ *
+ * A set, not a single row, and that is the whole reason Protocol 4 grew a per-member
+ * rank: the guild board carries one `leader_serial`, so before this the website could
+ * only ever be told about one leader, while a UO guild routinely has several.
+ *
+ * A NULL rank is excluded by the comparison, which is correct — the shard withholds
+ * the rank for a staff account rather than publishing the Leader its getter falsely
+ * reports, and "not known" must not be read as "leads this guild".
+ */
+const listGuildLeaders = (guildId) =>
+  query(
+    'SELECT serial FROM shard_guild_members WHERE guild_id = ? AND `rank` >= ? ORDER BY name ASC',
+    [guildId, LEADER_RANK],
+  )
+
+module.exports = { listGuilds, findGuild, listGuildMembers, listGuildLeaders, LEADER_RANK }

server/model/teamProvider/teamProvider.model.js

@@ -24,9 +24,27 @@ const core = require('../../core')
 const db = require('./teamProvider.db')
 const uoLinkConfig = require('../uoLinkConfig/uoLinkConfig.model')
 const uoLinkSocket = require('../../utils/uoLinkSocket')
+const clilocs = require('../shardClilocs/shardClilocs.model')
 
 const log = core.logger('teams')
 
+/**
+ * ServUO's five stock rank names, by the cliloc id the game names them with.
+ *
+ * A fallback, not the source of truth: the operator's own cliloc table is consulted
+ * first, and a shard with custom rank definitions sends a literal string that beats
+ * both. This exists because the cliloc table is populated only if someone ran the
+ * client-file extraction, and a roster on a shard that has not should still say
+ * "Warlord" rather than nothing.
+ */
+const STANDARD_RANK_NAMES = {
+  1062959: 'Leader',
+  1062960: 'Warlord',
+  1062961: 'Emissary',
+  1062962: 'Member',
+  1062963: 'Ronin',
+}
+
 /** A refusal, in the shape core reads (§2.3). */
 const refuse = (reason) => ({ ok: false, reason })
 
@@ -116,17 +134,19 @@ async function getTeamMembers(externalId) {
       return refuse(`roster for guild ${externalId} has not arrived yet (board says ${guild.members} members)`)
     }
 
-    const leaderSerial = guild.leader_serial || null
+    const labels = await rankLabels(rows)
     return {
       ok: true,
       complete: true,
       members: rows.map((row) => ({
         memberKey: row.serial,
         displayName: row.name || null,
-        // Not on the wire. The roster member is the standard actor object, which
+        rankLabel: labels.get(row.serial) || null,
-        // carries no guild rank — see the note at the bottom of this file.
+        // Rank 4 is Leader, and several members can hold it. A NULL rank is not a
-        rankLabel: null,
+        // leader: the shard withholds the rank for a staff account rather than
-        leader: Boolean(leaderSerial && row.serial === leaderSerial),
+        // publishing the Leader its getter falsely reports, and "not known" must
+        // never be read as "leads this guild".
+        leader: Number.isInteger(row.rank) && row.rank >= db.LEADER_RANK,
         online: Boolean(row.is_online),
         userId: resolveUserId(row),
       })),
@@ -138,17 +158,17 @@ async function getTeamMembers(externalId) {
 }
 
 /**
- * `getTeamLeaders(externalId)` — who leads the guild.
+ * `getTeamLeaders(externalId)` — everyone at leader rank.
  *
- * **One leader, because that is all the wire carries.** TEAMS.md §2.5 expects
+ * **All of them, which is why Protocol 4 grew a per-member rank.** The guild board
- * multiple leaders to be the normal case, from `PlayerMobile.GuildRank.Rank >= 4`,
+ * carries one `leader_serial`, so before the rank amendment this could only ever
- * and core supports them — but Protocol 4's roster member is the standard actor
+ * name a single member, while a UO guild routinely has several at rank 4 and
- * object with no rank field, so the only leadership this module can see is the
+ * TEAMS.md §2.5 treats multiple leaders as the normal case.
- * board's single `leader_serial` from `guild.update`. Reporting a guessed second
- * leader would be worse than reporting one honestly.
  *
- * Raising this to the full set is a protocol change (rank on the actor object),
+ * The board's own `leader_serial` is folded in as a floor. It is the guild's
- * not something this file can fix.
+ * founder-leader and it comes from a different frame (`guild.update`), so on a
+ * shard whose roster has not been re-emitted since the amendment it is the only
+ * leadership signal there is — and it should never be *lost* by moving to ranks.
  */
 async function getTeamLeaders(externalId) {
   const ready = await boardIsCurrent()
@@ -157,13 +177,65 @@ async function getTeamLeaders(externalId) {
   try {
     const [guild] = await db.findGuild(externalId)
     if (!guild) return refuse(`guild ${externalId} is not on the board`)
-    return { ok: true, leaders: guild.leader_serial ? [guild.leader_serial] : [] }
+
+    const rows = await db.listGuildLeaders(externalId)
+    const leaders = rows.map((r) => r.serial)
+
+    if (guild.leader_serial && !leaders.includes(guild.leader_serial)) {
+      leaders.push(guild.leader_serial)
+    }
+    return { ok: true, leaders }
   } catch (err) {
     log.warn('getTeamLeaders failed', { externalId, message: err.message })
     return refuse(`leadership unreadable: ${err.message}`)
   }
 }
 
+/**
+ * Resolve each member's rank to a display label, keyed by serial.
+ *
+ * The shard sends the rank's NAME as the game states it — a cliloc id for the five
+ * standard ranks, or a literal string for a custom rank definition — and never a
+ * resolved label, because ServUO ships no text for those clilocs. This module does
+ * have a cliloc table, which is why the resolution belongs here.
+ *
+ * Three sources, in order: a custom string wins, then the operator's cliloc table,
+ * then the five standard names. The last exists because the cliloc table is
+ * populated only if someone ran the client extraction, and a shard that has not
+ * should still read "Warlord" rather than nothing.
+ *
+ * Never throws: a rank label is decoration on a roster, and a lookup failure must
+ * not turn a good roster into a refusal.
+ */
+async function rankLabels(rows) {
+  const out = new Map()
+  const wanted = []
+
+  for (const row of rows) {
+    if (row.rank_name) {
+      out.set(row.serial, row.rank_name)
+    } else if (Number.isInteger(row.rank_cliloc)) {
+      wanted.push(row.rank_cliloc)
+    }
+  }
+
+  let resolved = new Map()
+  if (wanted.length) {
+    try {
+      resolved = await clilocs.resolveMany(wanted)
+    } catch (err) {
+      log.warn('rank cliloc lookup failed; falling back to the standard names', { message: err.message })
+    }
+  }
+
+  for (const row of rows) {
+    if (out.has(row.serial) || !Number.isInteger(row.rank_cliloc)) continue
+    const label = resolved.get(row.rank_cliloc) || STANDARD_RANK_NAMES[row.rank_cliloc] || null
+    if (label) out.set(row.serial, label)
+  }
+  return out
+}
+
 /**
  * The site account behind a character, or null.
  *

server/test/teamProvider.test.js

@@ -26,6 +26,7 @@ core.init({
 const db = require('../model/teamProvider/teamProvider.db')
 const uoLinkConfig = require('../model/uoLinkConfig/uoLinkConfig.model')
 const uoLinkSocket = require('../utils/uoLinkSocket')
+const clilocs = require('../model/shardClilocs/shardClilocs.model')
 const provider = require('../model/teamProvider/teamProvider.model')
 
 const saved = []
@@ -39,6 +40,10 @@ function patch(mod, name, fn) {
 function healthy() {
   patch(uoLinkConfig, 'getSafe', async () => ({ baseUrl: 'http://127.0.0.1:7787', enabled: true }))
   patch(uoLinkSocket, 'getState', () => ({ connected: true, running: true }))
+  // An operator who has never run the client extraction — the default. The standard
+  // rank names must still resolve from the fallback table.
+  patch(clilocs, 'resolveMany', async () => new Map())
+  patch(db, 'listGuildLeaders', async () => [])
 }
 
 const guild = (extra = {}) => ({
@@ -48,6 +53,7 @@ const guild = (extra = {}) => ({
 
 const member = (extra = {}) => ({
   serial: '0x1', name: 'Aldric', acct: 'aldric', web_id: null, is_player: 1,
+  rank: 1, rank_cliloc: 1062962, rank_name: null,
   linked_user_id: null, is_online: 0, ...extra,
 })
 
@@ -167,8 +173,8 @@ test('the external id is a string, so core never compares a number to one', asyn
 test('a roster maps to the member shape core expects', async () => {
   patch(db, 'findGuild', async () => [guild()])
   patch(db, 'listGuildMembers', async () => [
-    member({ serial: '0x1', name: 'Aldric', is_online: 1 }),
+    member({ serial: '0x1', name: 'Aldric', rank: 4, rank_cliloc: 1062959, is_online: 1 }),
-    member({ serial: '0x2', name: 'Bree', acct: null, is_online: 0 }),
+    member({ serial: '0x2', name: 'Bree', acct: null, rank: 1, is_online: 0 }),
   ])
 
   const { members } = await provider.getTeamMembers('1')
@@ -176,21 +182,113 @@ test('a roster maps to the member shape core expects', async () => {
   assert.equal(members[0].memberKey, '0x1')
   assert.equal(members[0].displayName, 'Aldric')
   assert.equal(members[0].online, true)
-  assert.equal(members[0].leader, true, 'matches the board’s leader_serial')
+  assert.equal(members[0].leader, true, 'rank 4 is Leader')
   assert.equal(members[1].leader, false)
   assert.equal(members[1].online, false)
 })
 
-test('rankLabel is null, honestly — the wire carries no guild rank', async () => {
+// ── Rank (the Protocol 4 amendment) ────────────────────────────────────────
-  // The roster member is the standard actor object (serial, name, player, acct?,
+
-  // webId?). Inventing a rank from the leader flag would be core displaying a
+test('several members can be leaders at once', async () => {
-  // label this module made up.
+  // The whole reason the wire grew a per-member rank: the board carries one
+  // leader_serial, so before this only a single leader could ever be reported.
   patch(db, 'findGuild', async () => [guild()])
-  patch(db, 'listGuildMembers', async () => [member()])
+  patch(db, 'listGuildMembers', async () => [
+    member({ serial: '0x1', rank: 4 }),
+    member({ serial: '0x2', rank: 4 }),
+    member({ serial: '0x3', rank: 3 }),
+  ])
+
   const { members } = await provider.getTeamMembers('1')
+  assert.deepEqual(members.filter((m) => m.leader).map((m) => m.memberKey), ['0x1', '0x2'])
+})
+
+test('getTeamLeaders returns everyone at rank 4, not just the board’s one', async () => {
+  patch(db, 'findGuild', async () => [guild({ leader_serial: '0x1' })])
+  patch(db, 'listGuildLeaders', async () => [{ serial: '0x1' }, { serial: '0x2' }])
+  assert.deepEqual((await provider.getTeamLeaders('1')).leaders, ['0x1', '0x2'])
+})
+
+test('the board’s leader is kept even when no roster row has rank yet', async () => {
+  // A shard whose roster has not been re-emitted since the amendment has no ranks
+  // stored. The founder-leader comes from a different frame and must not be lost
+  // by moving to ranks.
+  patch(db, 'findGuild', async () => [guild({ leader_serial: '0x9' })])
+  patch(db, 'listGuildLeaders', async () => [])
+  assert.deepEqual((await provider.getTeamLeaders('1')).leaders, ['0x9'])
+})
+
+test('the board’s leader is not duplicated when they also hold rank 4', async () => {
+  patch(db, 'findGuild', async () => [guild({ leader_serial: '0x1' })])
+  patch(db, 'listGuildLeaders', async () => [{ serial: '0x1' }, { serial: '0x2' }])
+  const { leaders } = await provider.getTeamLeaders('1')
+  assert.equal(new Set(leaders).size, leaders.length)
+})
+
+test('a NULL rank is not a leader — "not known" is not "leads this guild"', async () => {
+  // The shard withholds the rank for a staff account, because ServUO's GuildRank
+  // getter reports Leader for anyone at GameMaster or above whatever their real
+  // rank. Reading the absence as leadership would republish exactly that lie.
+  patch(db, 'findGuild', async () => [guild()])
+  patch(db, 'listGuildMembers', async () => [member({ serial: '0x1', rank: null, rank_cliloc: null })])
+
+  const { members } = await provider.getTeamMembers('1')
+  assert.equal(members[0].leader, false)
   assert.equal(members[0].rankLabel, null)
 })
 
+test('a standard rank resolves to its name without a cliloc table', async () => {
+  // The operator may never have run the client extraction, and a roster should
+  // still read "Warlord" rather than nothing.
+  patch(db, 'findGuild', async () => [guild()])
+  patch(db, 'listGuildMembers', async () => [
+    member({ serial: '0x1', rank: 4, rank_cliloc: 1062959 }),
+    member({ serial: '0x2', rank: 3, rank_cliloc: 1062960 }),
+    member({ serial: '0x3', rank: 0, rank_cliloc: 1062963 }),
+  ])
+
+  const { members } = await provider.getTeamMembers('1')
+  assert.deepEqual(members.map((m) => m.rankLabel), ['Leader', 'Warlord', 'Ronin'])
+})
+
+test('the operator’s cliloc table wins over the built-in names', async () => {
+  // A localised or edited client should name the ranks, not this module's English
+  // fallback.
+  patch(clilocs, 'resolveMany', async () => new Map([[1062960, 'Kriegsherr']]))
+  patch(db, 'findGuild', async () => [guild()])
+  patch(db, 'listGuildMembers', async () => [member({ serial: '0x1', rank: 3, rank_cliloc: 1062960 })])
+
+  assert.equal((await provider.getTeamMembers('1')).members[0].rankLabel, 'Kriegsherr')
+})
+
+test('a custom rank’s literal name beats both', async () => {
+  // A shard that replaced RankDefinition.Ranks sends a string instead of a cliloc,
+  // and its own naming has to survive.
+  patch(clilocs, 'resolveMany', async () => new Map([[1062960, 'Warlord']]))
+  patch(db, 'findGuild', async () => [guild()])
+  patch(db, 'listGuildMembers', async () => [
+    member({ serial: '0x1', rank: 3, rank_cliloc: 1062960, rank_name: 'Sword-Captain' }),
+  ])
+
+  assert.equal((await provider.getTeamMembers('1')).members[0].rankLabel, 'Sword-Captain')
+})
+
+test('a failing cliloc lookup falls back rather than failing the roster', async () => {
+  patch(clilocs, 'resolveMany', async () => { throw new Error('cliloc table missing') })
+  patch(db, 'findGuild', async () => [guild()])
+  patch(db, 'listGuildMembers', async () => [member({ serial: '0x1', rank: 3, rank_cliloc: 1062960 })])
+
+  const answer = await provider.getTeamMembers('1')
+  assert.equal(answer.ok, true, 'a label is decoration; losing it must not lose the roster')
+  assert.equal(answer.members[0].rankLabel, 'Warlord')
+})
+
+test('an unknown cliloc leaves the label null rather than inventing one', async () => {
+  patch(db, 'findGuild', async () => [guild()])
+  patch(db, 'listGuildMembers', async () => [member({ serial: '0x1', rank: 2, rank_cliloc: 9999999 })])
+  assert.equal((await provider.getTeamMembers('1')).members[0].rankLabel, null)
+})
+
 test('a member with no account at all is fine and unlinked', async () => {
   // §2.3 of the protocol spec: acct is genuinely optional — a PlayerMobile can
   // have no Account, and the local test world contains such mobiles.
@@ -221,16 +319,9 @@ test('web_id arrives as a string from the wire and is coerced', async () => {
   assert.equal(typeof members[0].userId, 'number')
 })
 
-test('leadership is the board’s single leader — one, and honestly one', async () => {
+test('a guild with no leader anywhere reports none rather than guessing', async () => {
-  // TEAMS.md §2.5 expects multiple leaders (GuildRank.Rank >= 4) and core
-  // supports them, but Protocol 4 does not put rank on the wire. Raising this to
-  // the full set is a protocol change, not something this file can fix.
-  patch(db, 'findGuild', async () => [guild({ leader_serial: '0x1' })])
-  assert.deepEqual((await provider.getTeamLeaders('1')).leaders, ['0x1'])
-})
-
-test('a guild with no leader on the board reports none rather than guessing', async () => {
   patch(db, 'findGuild', async () => [guild({ leader_serial: null })])
+  patch(db, 'listGuildLeaders', async () => [])
   const answer = await provider.getTeamLeaders('1')
   assert.equal(answer.ok, true)
   assert.deepEqual(answer.leaders, [])