diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 8c09cce..add43e5 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -86,9 +86,13 @@ jobs: - name: Build client run: npm run build --prefix client - bot-install: - # No tests/build to run; a clean install still catches a broken or - # out-of-sync lockfile before it ships in the bot image. + bot-tests: + # The install still runs first and still catches a broken or out-of-sync + # lockfile before it ships in the bot image — that was this job's whole + # purpose until phase 7 (TEAMS.md §7.1) put real logic in the bot: it now + # pulls slash-command definitions from the app, merges them into the + # whole-set PUT, and runs the defer→dispatch→edit path. None of that is + # reachable from the server suite, and phases 8 and 9 add more of it. runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 @@ -99,3 +103,7 @@ jobs: cache-dependency-path: bot/package-lock.json - name: Install bot deps run: npm ci --prefix bot + - name: Run bot tests + # Node's built-in runner, no browser and no Discord connection — the + # interaction is a fake that records what was called on it. + run: npm test --prefix bot diff --git a/bot/package.json b/bot/package.json index 7e97402..0c4692a 100644 --- a/bot/package.json +++ b/bot/package.json @@ -6,6 +6,7 @@ "main": "src/server.js", "scripts": { "start": "node src/server.js", + "test": "node --test test/*.test.js", "dev": "nodemon src/server.js" }, "keywords": ["discord", "discord.js"], diff --git a/bot/src/discord/discordManager.js b/bot/src/discord/discordManager.js index 5edc42a..c1b0742 100644 --- a/bot/src/discord/discordManager.js +++ b/bot/src/discord/discordManager.js @@ -5,6 +5,7 @@ const { Client, GatewayIntentBits, REST, Routes } = require('discord.js') const createLogger = require('../utils/logger') const commands = require('./commands') +const dynamicCommands = require('./dynamicCommands') const messageFilter = require('./messageFilter') const scheduler = require('../scheduler/scheduler') const roleMenuHandler = require('./roleMenuHandler') @@ -22,12 +23,46 @@ let status = 'disconnected' // disconnected | connecting | connected | error let statusDetail = null let lastConnectedAt = null +// One whole-set PUT of the bot's own commands plus whatever the app has +// registered (TEAMS.md §7.1). Because it replaces the set rather than adding to +// it, DEREGISTRATION is free: a module that is gone is simply absent from the +// next pull, and nobody has to remember to take its command back. async function registerCommands(applicationId, targetGuildId) { + const dynamic = dynamicCommands.definitions() const rest = new REST({ version: '10' }).setToken(client.token) await rest.put(Routes.applicationGuildCommands(applicationId, targetGuildId), { - body: commands.all.map((c) => c.data), + body: [...commands.all.map((c) => c.data), ...dynamic], }) - log.info('registered guild slash commands', { guildId: targetGuildId, count: commands.all.length }) + log.info('registered guild slash commands', { + guildId: targetGuildId, + builtIn: commands.all.length, + fromApp: dynamic.length, + }) +} + +/** + * Re-pull the app's commands and re-register the set if it moved. + * + * Called on `ready` and again whenever the app nudges + * (`POST /internal/refresh-commands`). A no-op when nothing changed, so a nudge + * per module state change costs one cheap GET rather than a REST.put per + * install — and a disconnected bot does nothing at all, since there is no + * application to register against until it logs in. + */ +async function refreshCommands() { + const result = await dynamicCommands.pull() + if (!result.ok || !result.changed) return result + if (!client || !client.isReady()) return result + try { + await registerCommands(client.application.id, guildId) + } catch (err) { + // The PUT is all-or-nothing: a definition Discord rejects costs every + // command, the built-ins included. Loud, and never fatal to the process. + log.error('re-registering slash commands failed — the previous set is still live', { + message: err.message, + }) + } + return result } async function stop() { @@ -54,6 +89,11 @@ async function stop() { // failure here leaves the client connected but flags an error status. async function onReady() { try { + // Pull BEFORE the single PUT, so the app's commands are in the very first + // registration rather than appearing a beat later. The pull never throws — + // an unreachable app costs the module commands and nothing else, and the + // bot's own set registers exactly as it always did. + await dynamicCommands.pull() await registerCommands(client.application.id, guildId) await scheduler.start(client) tempRoleSweeper.start(client) @@ -70,14 +110,19 @@ async function onReady() { } } -// Route an interaction: role-menu handler first, then chat-input slash commands. +// Route an interaction: role-menu handler first, then chat-input slash commands +// — the bot's own, then the app's. Built-ins are consulted FIRST and the pull +// already drops any module name that collides with one, so the two orderings +// agree; checking here as well means a name that somehow reached Discord twice +// still runs the bot's version rather than whichever registry answered first. async function onInteractionCreate(interaction) { if (await roleMenuHandler.handleInteraction(interaction)) return if (!interaction.isChatInputCommand()) return const command = commands.get(interaction.commandName) - if (!command) return + if (!command && !dynamicCommands.has(interaction.commandName)) return try { - await command.execute(interaction) + if (command) await command.execute(interaction) + else await dynamicCommands.execute(interaction) } catch (err) { log.error('command execution failed', { command: interaction.commandName, message: err.message }) const payload = { content: 'Something went wrong running that command.', ephemeral: true } @@ -146,4 +191,4 @@ function getConnection() { return { client, guildId } } -module.exports = { start, stop, getStatus, getConnection } +module.exports = { start, stop, getStatus, getConnection, refreshCommands } diff --git a/bot/src/discord/dynamicCommands.js b/bot/src/discord/dynamicCommands.js new file mode 100644 index 0000000..3632df1 --- /dev/null +++ b/bot/src/discord/dynamicCommands.js @@ -0,0 +1,242 @@ +// Slash commands whose DEFINITION and HANDLER live in the website process +// (TEAMS.md §7.1). The bot pulls the definitions, registers them alongside its +// own, and executes one by deferring, asking the app, and editing the reply in. +// +// Everything Discord-specific is here and nothing else is: the app's dispatcher +// resolves the actor, enforces access and produces a platform-neutral envelope, +// and this file turns that envelope into an interaction reply. A module never +// touches an interaction, which is what makes the registration API something a +// second platform could implement. +const { PermissionFlagsBits } = require('discord.js') + +const appInternal = require('../site/appInternalClient') +const staticCommands = require('./commands') +const createLogger = require('../utils/logger') + +const log = createLogger('dynamic-commands') + +// §7.1.1's four types, and the only four. The app rejects anything else at +// registration; this map is the second half of that agreement. +const OPTION_TYPE = { string: 3, integer: 4, boolean: 5, user: 6 } + +// The pulled set, and the app's module-state counter it came from. `null` +// version means "never successfully pulled", which is distinct from 0 ("pulled +// while the app had no modules loaded") — the first should retry, the second is +// a true answer. +let pulled = [] +let version = null + +/** + * Ask the app for the current definitions. + * + * **A failed pull KEEPS the previous set.** The app being briefly unreachable is + * not the same as it having no commands, and treating it as such would + * deregister every module command from Discord on a restart blip — then + * re-register them a minute later, with members watching commands appear and + * disappear. Nothing changes until the app actually answers. + * + * @returns {Promise<{ok: boolean, changed: boolean, count: number}>} + */ +async function pull() { + const res = await appInternal.fetchCommands() + if (!res.ok) { + log.warn('command pull failed — keeping the set already registered', { + error: res.error, + holding: pulled.length, + }) + return { ok: false, changed: false, count: pulled.length } + } + + const { version: pulledVersion, commands } = res.data || {} + const next = Array.isArray(commands) ? commands.filter(usable) : [] + const changed = version === null || pulledVersion !== version || next.length !== pulled.length + pulled = next + version = typeof pulledVersion === 'number' ? pulledVersion : 0 + return { ok: true, changed, count: pulled.length } +} + +/** + * Drop a pulled definition the bot cannot honour. + * + * **The name collision the app cannot see.** The app validates a command against + * everything IT has registered; it does not know the bot's own static array + * exists. A module registering `ping` would produce two `ping` entries in one + * `REST.put`, which Discord rejects as a batch — taking down every command + * including the bot's own. The bot's built-ins win, because they are the ones a + * module cannot be asked to change. + */ +function usable(definition) { + if (!definition || typeof definition.name !== 'string') return false + if (staticCommands.get(definition.name)) { + log.warn('module slash command collides with a built-in and is ignored', { + command: definition.name, + owner: definition.owner, + }) + return false + } + return true +} + +/** + * The pulled definitions as Discord command data, for the whole-set PUT. + * + * `access: 'staff'` becomes a Discord-side permission default; `linked` cannot + * be expressed in Discord's permission model at all — there is no "has a website + * account" predicate — so it is simply not advertised and the app's dispatcher + * refuses it. That asymmetry is the reason §7.1 says access is enforced twice + * and that only the server half is the gate. + */ +function definitions() { + return pulled.map((c) => { + const data = { + name: c.name, + description: c.description, + options: (c.options || []).map((o) => ({ + name: o.name, + description: o.description, + type: OPTION_TYPE[o.type], + required: Boolean(o.required), + ...(o.choices ? { choices: o.choices } : {}), + })), + } + if (c.access === 'staff') data.default_member_permissions = PermissionFlagsBits.ModerateMembers.toString() + return data + }) +} + +/** Is this a command the app owns? Asked before the static registry is consulted. */ +const has = (name) => pulled.some((c) => c.name === name) + +// Read the options the member actually supplied, by the names the definition +// declared. A `user` option is passed on as the Discord user id and nothing else +// — a handler receives platform ids, never a platform object. +function collectOptions(interaction, definition) { + const out = {} + for (const option of definition.options || []) { + const supplied = interaction.options.get(option.name) + if (supplied === null || supplied === undefined) continue + out[option.name] = option.type === 'user' ? String(supplied.value) : supplied.value + } + return out +} + +// What the caller sees when the app declined. The COPY lives here rather than in +// the app on purpose: the app answers with a machine reason, and how a refusal is +// phrased to a member is the platform's own voice. +function refusal({ reason, access }) { + if (reason === 'forbidden' && access === 'linked') { + return 'Link your Discord account on the site to use this command.' + } + if (reason === 'forbidden') return 'You do not have access to that command.' + if (reason === 'unknown') return 'That command is no longer available.' + return 'Something went wrong running that command.' +} + +// Envelope → interaction payload. A response with fields or a title is an embed; +// a bare `text` is plain content, which reads better for a one-line answer. +function render(envelope) { + const { text, title, fields, url } = envelope + if (!title && !fields) return { content: text || '​' } + const embed = {} + if (title) embed.title = title + if (text) embed.description = text + if (url) embed.url = url + if (fields) embed.fields = fields + return { embeds: [embed] } +} + +/** + * Deliver the envelope at the privacy the HANDLER asked for, not the privacy the + * deferral guessed. + * + * When the two agree — the ordinary case — this is one `editReply`. When the + * handler wants a private answer to a publicly deferred command, the deferred + * reply is deleted and the answer arrives as an ephemeral follow-up: the + * interaction token stays valid, so this is a supported path rather than a + * trick, and the cost is a "thinking…" that appears and vanishes. + * + * There is no reverse case. A command deferred ephemerally is one whose answers + * are all about the caller's own account, and nothing it returns should become + * public because a handler forgot a flag. + */ +async function reply(interaction, envelope, deferredEphemeral) { + const payload = render(envelope) + if (!envelope.ephemeral || deferredEphemeral) { + await interaction.editReply(payload) + return + } + await interaction.deleteReply() + await interaction.followUp({ ...payload, ephemeral: true }) +} + +/** + * Defer, dispatch, edit. + * + * **The deferral comes first, always.** Discord gives three seconds to acknowledge + * an interaction; the app is given four to answer. Deferring before the dispatch + * is what keeps the website out of that critical path entirely — a wedged handler + * costs its own reply and never an "application did not respond". + * + * A failure at any point after the defer is an edit, not a reply: the interaction + * has already been acknowledged, and `reply()` on a deferred interaction throws. + */ +async function execute(interaction) { + const definition = pulled.find((c) => c.name === interaction.commandName) + if (!definition) return false + + // **Ephemerality is fixed at the DEFERRAL, which happens before the answer + // exists.** That is Discord's rule, not a choice here, and it is the whole + // reason this needs care: the handler decides privacy per answer — a refusal + // is private, a guild summary is not — and by the time it says so the reply is + // already public or already not. + // + // So: defer for the common case (public, or private for a command that only + // ever speaks about the caller's own account), and if the envelope disagrees, + // reconcile below. Getting this wrong is not cosmetic — the live walk caught it + // posting "guild information is not shown to your account" into the channel, + // which announces a member's access level to everyone in it. + const ephemeral = definition.access === 'linked' + await interaction.deferReply({ ephemeral }) + + const res = await appInternal.dispatchCommand({ + command: definition.name, + options: collectOptions(interaction, definition), + platformUserId: interaction.user.id, + guildId: interaction.guildId, + }) + + // A transport failure and a handler failure are the same sentence to the + // member and different lines in the log: one is the app being unreachable, + // the other is a module's code. + // A refusal is ALWAYS private, whatever the command's usual privacy: "you do + // not have access to that" is about one member and belongs to one member. + if (!res.ok) { + log.warn('command dispatch failed', { command: definition.name, error: res.error }) + await reply(interaction, { text: refusal({ reason: 'error' }), ephemeral: true }, ephemeral) + return true + } + if (!res.data || !res.data.ok) { + await reply(interaction, { text: refusal(res.data || {}), ephemeral: true }, ephemeral) + return true + } + + const envelope = res.data.response || {} + await reply(interaction, envelope, ephemeral) + + // The private aside beside a public answer (§9 answer 5). Skipped when the + // reply was already private — the member would just be told the same thing + // twice, in the same place. + if (envelope.notice && !ephemeral && !envelope.ephemeral) { + await interaction.followUp({ content: envelope.notice, ephemeral: true }) + } + return true +} + +// Test-only: the pulled set is process-global, so a test that pulls has to be +// able to hand the process back. +function _reset() { + pulled = [] + version = null +} + +module.exports = { pull, definitions, has, execute, _reset } diff --git a/bot/src/internal/internal.controller.js b/bot/src/internal/internal.controller.js index 01fe2dd..c55e286 100644 --- a/bot/src/internal/internal.controller.js +++ b/bot/src/internal/internal.controller.js @@ -99,4 +99,26 @@ async function reverseModAction(req, res) { } } -module.exports = { setConfig, getStatus: getStatusHandler, announce, reverseModAction } +// POST /internal/refresh-commands — the app's nudge that its registered +// slash-command set has moved (TEAMS.md §7.1). No body: the bot re-pulls +// `/internal/commands` and re-registers only if the set actually changed, so the +// nudge stays a cheap thing the app can send on every module state change. +// +// Deliberately its OWN endpoint rather than riding on /internal/config, which +// carries the decrypted bot token: saying "commands changed" should not require +// the app to read a secret out of the database. +// +// Answers 200 even when disconnected — there is no application to register +// against until the bot logs in, and `ready` pulls again anyway. A 5xx here +// would make an ordinary module install look like a failure in the admin panel. +async function refreshCommands(req, res) { + try { + const result = await discordManager.refreshCommands() + return res.json({ ok: true, ...result }) + } catch (err) { + log.error('refresh-commands failed', { message: err.message }) + return res.json({ ok: false, error: err.message }) + } +} + +module.exports = { setConfig, getStatus: getStatusHandler, announce, reverseModAction, refreshCommands } diff --git a/bot/src/internal/internal.routes.js b/bot/src/internal/internal.routes.js index 49891e4..8d2f04c 100644 --- a/bot/src/internal/internal.routes.js +++ b/bot/src/internal/internal.routes.js @@ -11,5 +11,6 @@ router.post('/config', ctrl.setConfig) router.get('/status', ctrl.getStatus) router.post('/announce', ctrl.announce) router.post('/mod-reverse', ctrl.reverseModAction) +router.post('/refresh-commands', ctrl.refreshCommands) module.exports = router diff --git a/bot/src/site/appInternalClient.js b/bot/src/site/appInternalClient.js new file mode 100644 index 0000000..3f3e07a --- /dev/null +++ b/bot/src/site/appInternalClient.js @@ -0,0 +1,76 @@ +// Shared-secret client for the APP's internal listener (port 3001) — the +// bot→app direction of the channel `botInternalClient.js` runs app→bot. +// +// Two callers, both slash-command plumbing (TEAMS.md §7.1): pull the registered +// command definitions, and dispatch one that a member has just run. Distinct +// from siteApiClient.js, which reads the site's PUBLIC API with no secret at all. +// +// **The base URL is derived from `SITE_INTERNAL_URL`'s origin, not configured +// separately.** That variable already points at the app's internal listener — +// `http://app:3001/internal/bot-config` — and adding a second variable naming the +// same host would be one more thing an operator can get half-right. Deriving it +// means every existing deployment gains these endpoints with no compose change. +const createLogger = require('../utils/logger') + +const log = createLogger('app-internal') + +const KEY = process.env.BOT_INTERNAL_KEY || '' + +// §7.1's budget, and the same 4s `botInternalClient` uses in the other +// direction. The app bounds its own handlers UNDER this (3s), so a timeout here +// normally means the app itself is unreachable rather than a module being slow. +const TIMEOUT_MS = 4000 + +function baseUrl() { + const configured = process.env.SITE_INTERNAL_URL + if (!configured) return null + try { + return new URL(configured).origin + } catch { + log.error('SITE_INTERNAL_URL is not a URL — slash-command registration is off', { configured }) + return null + } +} + +async function call(path, { method = 'GET', body } = {}) { + const base = baseUrl() + if (!base || !KEY) return { ok: false, error: 'SITE_INTERNAL_URL or BOT_INTERNAL_KEY not set' } + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), TIMEOUT_MS) + try { + const res = await fetch(`${base}${path}`, { + method, + headers: { 'Content-Type': 'application/json', 'X-Internal-Key': KEY }, + body: body ? JSON.stringify(body) : undefined, + signal: controller.signal, + }) + if (!res.ok) return { ok: false, status: res.status, error: `app responded ${res.status}` } + return { ok: true, status: res.status, data: await res.json() } + } catch (err) { + log.warn('app internal call failed', { path, message: err.message }) + return { ok: false, status: 0, error: err.message } + } finally { + clearTimeout(timeout) + } +} + +/** The registered slash-command definitions, plus the version they belong to. */ +function fetchCommands() { + return call('/internal/commands') +} + +/** + * Run one command in the app and get the response envelope back. + * + * The bot has already deferred by the time this is called, so the only deadline + * that matters is Discord's 15-minute follow-up window — TIMEOUT_MS is about not + * holding an interaction open on a wedged app, not about the 3-second ack. + */ +function dispatchCommand({ command, options, platformUserId, guildId }) { + return call('/internal/commands/dispatch', { + method: 'POST', + body: { command, options, platform: 'discord', platformUserId, guildId }, + }) +} + +module.exports = { fetchCommands, dispatchCommand } diff --git a/bot/test/appInternalClient.test.js b/bot/test/appInternalClient.test.js new file mode 100644 index 0000000..9d3ae9b --- /dev/null +++ b/bot/test/appInternalClient.test.js @@ -0,0 +1,77 @@ +// The bot→app internal client (TEAMS.md §7.1). +// +// One property carries this file: the base URL is DERIVED from +// `SITE_INTERNAL_URL`, which already names the app's internal listener with a +// path on the end. That derivation is the reason every existing deployment gains +// slash commands with no compose change, and it is exactly the kind of string +// handling that breaks silently — a wrong base means "the app is down" forever, +// with nothing in the logs but a fetch error. + +const { test, beforeEach, afterEach } = require('node:test') +const assert = require('node:assert/strict') + +const env = { ...process.env } +const realFetch = global.fetch + +beforeEach(() => { + process.env.SITE_INTERNAL_URL = 'http://app:3001/internal/bot-config' + process.env.BOT_INTERNAL_KEY = 'shh' + delete require.cache[require.resolve('../src/site/appInternalClient')] +}) + +afterEach(() => { + process.env = { ...env } + global.fetch = realFetch +}) + +/** Load the client fresh and record the single fetch it makes. */ +function withFetch(response) { + const seen = {} + global.fetch = async (url, init) => { + seen.url = url + seen.init = init + return response + } + // eslint-disable-next-line global-require + return { client: require('../src/site/appInternalClient'), seen } +} + +const ok = (body) => ({ ok: true, status: 200, json: async () => body }) + +test('the commands URL is the internal listener’s origin, not its bot-config path', async () => { + const { client, seen } = withFetch(ok({ version: 3, commands: [] })) + const res = await client.fetchCommands() + assert.equal(seen.url, 'http://app:3001/internal/commands') + assert.equal(seen.init.headers['X-Internal-Key'], 'shh') + assert.deepEqual(res.data, { version: 3, commands: [] }) +}) + +test('a dispatch names the platform, so the app never has to guess', async () => { + const { client, seen } = withFetch(ok({ ok: true, response: {} })) + await client.dispatchCommand({ command: 'guild', options: { name: 'KOC' }, platformUserId: '5', guildId: '9' }) + assert.equal(seen.url, 'http://app:3001/internal/commands/dispatch') + assert.deepEqual(JSON.parse(seen.init.body), { + command: 'guild', options: { name: 'KOC' }, platform: 'discord', platformUserId: '5', guildId: '9', + }) +}) + +// A bot with no internal URL configured is an ordinary deployment state (the +// warning already exists in bootstrap.js); it must not become an exception on +// every `ready`. +test('an unconfigured or unparseable SITE_INTERNAL_URL is a refusal, not a throw', async () => { + delete process.env.SITE_INTERNAL_URL + const { client } = withFetch(ok({})) + assert.equal((await client.fetchCommands()).ok, false) + + delete require.cache[require.resolve('../src/site/appInternalClient')] + process.env.SITE_INTERNAL_URL = 'not a url' + // eslint-disable-next-line global-require + assert.equal((await require('../src/site/appInternalClient').fetchCommands()).ok, false) +}) + +test('a non-2xx carries its status so the caller can tell "down" from "rejected"', async () => { + const { client } = withFetch({ ok: false, status: 401, json: async () => ({}) }) + const res = await client.fetchCommands() + assert.equal(res.ok, false) + assert.equal(res.status, 401) +}) diff --git a/bot/test/dynamicCommands.test.js b/bot/test/dynamicCommands.test.js new file mode 100644 index 0000000..a7882b4 --- /dev/null +++ b/bot/test/dynamicCommands.test.js @@ -0,0 +1,269 @@ +// ── The bot's half of module slash commands (TEAMS.md §7.1) ──────────────── +// +// The first tests in this package, and they exist for a specific reason: phases +// 8 and 9 put more of the Discord integration in this process, and the failure +// modes here are ones no unit test in `server/` can see — a whole-set PUT that +// one bad entry poisons, a deferral that has to happen before anything slow, and +// a reply that must be EDITED rather than sent once the interaction is deferred. +// +// Nothing here talks to Discord. `interaction` is a fake that records what was +// called on it, which is the whole of what this file is asserting about. + +const { test, beforeEach } = require('node:test') +const assert = require('node:assert/strict') + +const dynamic = require('../src/discord/dynamicCommands') +const appInternal = require('../src/site/appInternalClient') +const staticCommands = require('../src/discord/commands') + +const originals = { + fetchCommands: appInternal.fetchCommands, + dispatchCommand: appInternal.dispatchCommand, + get: staticCommands.get, +} + +beforeEach(() => { + dynamic._reset() + Object.assign(appInternal, originals) + staticCommands.get = originals.get +}) + +const definition = (over = {}) => ({ + name: 'guild', + description: 'Show a guild', + owner: 'uo', + access: 'everyone', + options: [{ name: 'name', type: 'string', description: 'Guild name', required: false }], + ...over, +}) + +const answers = (commands, version = 1) => { + appInternal.fetchCommands = async () => ({ ok: true, data: { version, commands } }) +} + +function fakeInteraction({ commandName = 'guild', options = {}, userId = '555' } = {}) { + const calls = [] + return { + calls, + commandName, + guildId: '999', + user: { id: userId }, + options: { + get: (name) => (name in options ? { value: options[name] } : null), + }, + deferReply: async (payload) => calls.push(['defer', payload]), + editReply: async (payload) => calls.push(['edit', payload]), + deleteReply: async () => calls.push(['delete']), + followUp: async (payload) => calls.push(['followUp', payload]), + } +} + +// ── Pulling ──────────────────────────────────────────────────────────────── + +test('a pull reports whether the set moved, so a nudge is cheap', async () => { + answers([definition()], 7) + assert.deepEqual(await dynamic.pull(), { ok: true, changed: true, count: 1 }) + // Same version, same size: nothing to re-register, and re-registering anyway + // would mean a REST.put per module state change instead of per real change. + assert.deepEqual(await dynamic.pull(), { ok: true, changed: false, count: 1 }) + answers([definition()], 8) + assert.equal((await dynamic.pull()).changed, true) +}) + +// Otherwise a restart blip would deregister every module command from Discord +// and re-register it a minute later, with members watching it happen. +test('a failed pull keeps the set already registered', async () => { + answers([definition()]) + await dynamic.pull() + appInternal.fetchCommands = async () => ({ ok: false, error: 'ECONNREFUSED' }) + assert.deepEqual(await dynamic.pull(), { ok: false, changed: false, count: 1 }) + assert.equal(dynamic.definitions().length, 1) +}) + +// The collision the app cannot see: it validates against what IT registered and +// does not know the bot's own array exists. Two entries of one name in a single +// PUT is rejected as a batch, taking the built-ins down with it. +test('a module command that collides with a built-in is dropped, not registered', async () => { + staticCommands.get = (name) => (name === 'ping' ? { data: { name: 'ping' } } : undefined) + answers([definition({ name: 'ping' }), definition()]) + await dynamic.pull() + assert.deepEqual(dynamic.definitions().map((d) => d.name), ['guild']) + assert.equal(dynamic.has('ping'), false) +}) + +test('definitions carry Discord’s numeric option types, not the contract’s names', async () => { + answers([definition({ + options: [ + { name: 'who', type: 'user', description: 'A member', required: true }, + { name: 'n', type: 'integer', description: 'How many', choices: [{ name: 'one', value: 1 }] }, + ], + })]) + await dynamic.pull() + const [data] = dynamic.definitions() + assert.deepEqual(data.options.map((o) => o.type), [6, 4]) + assert.deepEqual(data.options[1].choices, [{ name: 'one', value: 1 }]) + assert.equal(data.default_member_permissions, undefined) +}) + +// `linked` has no Discord equivalent — there is no "has a website account" +// predicate — so only `staff` maps, and the app re-checks both regardless. +test('only access: staff becomes a Discord permission default', async () => { + answers([definition({ access: 'staff' }), definition({ name: 'other', access: 'linked' })]) + await dynamic.pull() + const [staff, linked] = dynamic.definitions() + assert.equal(typeof staff.default_member_permissions, 'string') + assert.equal(linked.default_member_permissions, undefined) +}) + +// ── Executing ────────────────────────────────────────────────────────────── + +test('the deferral happens before the dispatch, always', async () => { + answers([definition()]) + await dynamic.pull() + let deferredFirst = false + const interaction = fakeInteraction() + appInternal.dispatchCommand = async () => { + deferredFirst = interaction.calls.length === 1 && interaction.calls[0][0] === 'defer' + return { ok: true, data: { ok: true, response: { text: 'hi' } } } + } + await dynamic.execute(interaction) + assert.ok(deferredFirst, 'the website is never in Discord’s 3-second ack path') + assert.deepEqual(interaction.calls.at(-1), ['edit', { content: 'hi' }]) +}) + +test('the options the member supplied are passed by name, as plain values', async () => { + answers([definition({ + options: [ + { name: 'name', type: 'string', description: 'd' }, + { name: 'who', type: 'user', description: 'd' }, + { name: 'missing', type: 'string', description: 'd' }, + ], + })]) + await dynamic.pull() + let sent = null + appInternal.dispatchCommand = async (body) => { + sent = body + return { ok: true, data: { ok: true, response: {} } } + } + await dynamic.execute(fakeInteraction({ options: { name: 'KOC', who: '42' } })) + assert.deepEqual(sent.options, { name: 'KOC', who: '42' }) + assert.equal(sent.platformUserId, '555') + assert.equal(sent.guildId, '999') +}) + +test('a title or fields render as an embed; a bare text does not', async () => { + answers([definition()]) + await dynamic.pull() + appInternal.dispatchCommand = async () => ({ + ok: true, + data: { ok: true, response: { title: 'Knights', text: 'Alliance: Accord', fields: [{ name: 'Members', value: '12' }], url: 'https://site.test/uo/guilds/7' } }, + }) + const interaction = fakeInteraction() + await dynamic.execute(interaction) + const [, payload] = interaction.calls.at(-1) + assert.equal(payload.embeds[0].title, 'Knights') + assert.equal(payload.embeds[0].description, 'Alliance: Accord') + assert.equal(payload.embeds[0].url, 'https://site.test/uo/guilds/7') +}) + +// §9 answer 5: the public projection, plus a private nudge to link. One reply +// cannot be both, so the aside is a follow-up — which is the bot's decision to +// make, not the handler's. +test('a notice becomes an ephemeral follow-up beside a public answer', async () => { + answers([definition()]) + await dynamic.pull() + appInternal.dispatchCommand = async () => ({ + ok: true, + data: { ok: true, response: { text: 'public', notice: 'Link your account' } }, + }) + const interaction = fakeInteraction() + await dynamic.execute(interaction) + assert.deepEqual(interaction.calls.at(-1), ['followUp', { content: 'Link your account', ephemeral: true }]) +}) + +test('a notice is not repeated when the answer was already private', async () => { + answers([definition({ access: 'linked' })]) + await dynamic.pull() + appInternal.dispatchCommand = async () => ({ + ok: true, + data: { ok: true, response: { text: 'private', notice: 'Link your account' } }, + }) + const interaction = fakeInteraction() + await dynamic.execute(interaction) + assert.deepEqual(interaction.calls[0], ['defer', { ephemeral: true }]) + assert.equal(interaction.calls.some(([kind]) => kind === 'followUp'), false) +}) + +// Every failure path EDITS. Replying to a deferred interaction throws, so a +// refusal that used reply() would turn a clean "no" into an unhandled error. +// Ephemerality is fixed at the DEFERRAL, which happens before the handler has +// said anything — so honouring a per-answer flag needs the deferred reply +// withdrawn. The live walk caught the version that ignored it posting "guild +// information is not shown to your account" into the channel, which announces a +// member's access level to everyone in it. +test('a handler asking for privacy gets it, even though the deferral was public', async () => { + answers([definition()]) + await dynamic.pull() + appInternal.dispatchCommand = async () => ({ + ok: true, data: { ok: true, response: { text: 'just for you', ephemeral: true } }, + }) + const interaction = fakeInteraction() + await dynamic.execute(interaction) + assert.deepEqual(interaction.calls.map(([kind]) => kind), ['defer', 'delete', 'followUp']) + assert.deepEqual(interaction.calls.at(-1)[1], { content: 'just for you', ephemeral: true }) +}) + +test('an already-private deferral just edits — no second message', async () => { + answers([definition({ access: 'linked' })]) + await dynamic.pull() + appInternal.dispatchCommand = async () => ({ + ok: true, data: { ok: true, response: { text: 'private', ephemeral: true } }, + }) + const interaction = fakeInteraction() + await dynamic.execute(interaction) + assert.deepEqual(interaction.calls.map(([kind]) => kind), ['defer', 'edit']) +}) + +// "You do not have access to that" is about one member and belongs to one +// member, whatever the command's usual privacy. +test('a refusal is always private', async () => { + answers([definition()]) + await dynamic.pull() + appInternal.dispatchCommand = async () => ({ ok: true, data: { ok: false, reason: 'forbidden' } }) + const interaction = fakeInteraction() + await dynamic.execute(interaction) + assert.deepEqual(interaction.calls.map(([kind]) => kind), ['defer', 'delete', 'followUp']) + assert.equal(interaction.calls.at(-1)[1].ephemeral, true) +}) + +test('a refusal is phrased by the bot and edited into the deferred reply', async () => { + answers([definition({ access: 'linked' })]) + await dynamic.pull() + appInternal.dispatchCommand = async () => ({ + ok: true, data: { ok: false, reason: 'forbidden', access: 'linked', isLinked: false }, + }) + const interaction = fakeInteraction() + await dynamic.execute(interaction) + assert.match(interaction.calls.at(-1)[1].content, /Link your Discord account/) + // Deferred ephemerally (access: 'linked'), so the refusal is one edit and no + // withdrawal — replying twice to a deferred interaction is what throws. + assert.deepEqual(interaction.calls.map(([kind]) => kind), ['defer', 'edit']) +}) + +test('an unreachable app is the same sentence to the member and a different line in the log', async () => { + answers([definition()]) + await dynamic.pull() + appInternal.dispatchCommand = async () => ({ ok: false, error: 'timeout' }) + const interaction = fakeInteraction() + await dynamic.execute(interaction) + assert.match(interaction.calls.at(-1)[1].content, /Something went wrong/) + assert.equal(interaction.calls.at(-1)[1].ephemeral, true) +}) + +test('an interaction for a command the app no longer serves is left alone', async () => { + answers([definition()]) + await dynamic.pull() + const interaction = fakeInteraction({ commandName: 'gone' }) + assert.equal(await dynamic.execute(interaction), false) + assert.deepEqual(interaction.calls, [], 'nothing is deferred for a command that is not ours') +}) diff --git a/server/routes.guards.json b/server/routes.guards.json index 343ac05..e2f624d 100644 --- a/server/routes.guards.json +++ b/server/routes.guards.json @@ -2047,6 +2047,22 @@ "gates": [ "requireInternalKey" ] + }, + { + "method": "GET", + "path": "/internal/commands", + "handlers": 1, + "gates": [ + "requireInternalKey" + ] + }, + { + "method": "POST", + "path": "/internal/commands/dispatch", + "handlers": 1, + "gates": [ + "requireInternalKey" + ] } ] } diff --git a/server/routes.manifest.json b/server/routes.manifest.json index 30bf058..b18ddb8 100644 --- a/server/routes.manifest.json +++ b/server/routes.manifest.json @@ -842,6 +842,14 @@ { "method": "GET", "path": "/internal/bot-config" + }, + { + "method": "GET", + "path": "/internal/commands" + }, + { + "method": "POST", + "path": "/internal/commands/dispatch" } ] } diff --git a/server/src/model/teams/teamGrants.model.js b/server/src/model/teams/teamGrants.model.js index a1adf4b..8d2f446 100644 --- a/server/src/model/teams/teamGrants.model.js +++ b/server/src/model/teams/teamGrants.model.js @@ -157,6 +157,10 @@ async function forumGuests(teamId) { module.exports = { CAP_KEY, DEFAULT_CAP, + // Exported since phase 7: the Discord dispatcher's `access: 'staff'` has to + // mean the same two roles every other Team surface means by it, and a second + // copy of the list is a copy that drifts. + STAFF_ROLES, grantCap, authorityFor, grant, diff --git a/server/src/modules/lifecycle.js b/server/src/modules/lifecycle.js index 3e23a3d..3a9eea3 100644 --- a/server/src/modules/lifecycle.js +++ b/server/src/modules/lifecycle.js @@ -31,6 +31,33 @@ const log = require('../utils/logger')('modules') // such budget on purpose — it delays the listener binding, which is the feature. const SHUTDOWN_BUDGET_MS = 5000 +/** + * Nudge the bot to re-pull the slash-command set, from the two places that + * actually change it in a live process: a boot, and an operator disabling a + * module (which `remove` and `purge` both run through). + * + * Enabling and installing are deliberately NOT here — both ask for a restart + * before the module runs, and a command whose handler is not registered yet is a + * command that would answer "unknown". The nudge follows the state, not the + * intention. + * + * Required lazily, and deliberately NOT awaited by either caller: the bot is + * optional infrastructure, and neither a boot nor an operator's disable should + * wait out `botInternalClient`'s 4s timeout because a bot container is wedged. + * Nothing here throws — a failed nudge is a log line, and the bot re-pulls on its + * next `ready` regardless. + */ +async function nudgeBot(why) { + try { + // eslint-disable-next-line global-require + const bot = require('../utils/botInternalClient') + const res = await bot.refreshCommands() + if (!res.ok) log.info('bot did not take the slash-command nudge', { why, error: res.error }) + } catch (err) { + log.warn('slash-command nudge failed', { why, message: err.message }) + } +} + /** * Run one database call for one module without letting it become everyone's * failure. Returns null on failure, having logged it. @@ -179,6 +206,14 @@ async function boot({ modules, model } = {}) { // is a stale projection, never a site that will not start. // eslint-disable-next-line global-require await safe('starting the team reconciler', () => require('../model/teams/teamSync.model').start()) + + // Tell the bot the slash-command set may have moved (TEAMS.md §7.1). + // + // The bot pulls on its own `ready` too, so this is not the only path — it is + // the path for the case `ready` does not cover: the APP restarting while the + // bot stays connected, which is every ordinary redeploy. Without it, a module + // added in that deploy has no command until someone restarts the bot. + nudgeBot('boot') } /** Reject if `fn`'s promise has not settled within `ms`. */ @@ -280,6 +315,7 @@ async function stop(id, { modules, model, budgetMs = SHUTDOWN_BUDGET_MS } = {}) } await safe(`disabling module "${id}"`, () => rows.disable(id)) + nudgeBot(`disable:${id}`) return { stopped, error } } diff --git a/server/src/modules/loader.js b/server/src/modules/loader.js index 9d42d1e..0735782 100644 --- a/server/src/modules/loader.js +++ b/server/src/modules/loader.js @@ -282,13 +282,16 @@ function buildApi(record) { once('registerTeamProvider') record.staged.registerTeamProvider(provider) }, - // Declared in 1.6.0 with the rest of the Team surface; the bot half that - // executes a command lands in phase 7 (§7.1). Present and throwing rather - // than absent, so a module written against the published version fails at - // registration with a sentence naming the phase, instead of at whatever - // moment someone first types the command. - registerSlashCommands() { - throw new Error('api.registerSlashCommands is not available until Discord slash commands land (TEAMS.md §7.1)') + // Chat-platform slash commands (API 1.6.0, §7.1), live since phase 7. Like + // registerTeamProvider above, the handler this stages is core CALLING THE + // MODULE and waiting for an answer — but from further away than any other + // member: the caller is a bot in another container, holding a Discord + // interaction open on a deadline. `once` for the same reason the two + // registries above take it — a second call is a module changing its mind + // halfway through register(), not adding to what it already said. + registerSlashCommands(commands) { + once('registerSlashCommands') + record.staged.registerSlashCommands(commands) }, // The two lifecycle hooks (§2.5). Registered here, dispatched from // lifecycle.js — this file runs with no database and the hooks run with one. diff --git a/server/src/modules/registries.js b/server/src/modules/registries.js index 21a77a7..908b580 100644 --- a/server/src/modules/registries.js +++ b/server/src/modules/registries.js @@ -68,6 +68,18 @@ const postHooks = new Map() // second registration is a collision rather than an addition. let teamProvider = null +// command name → { owner, name, description, options, access, handler }. Slash +// commands a registrant has published for the chat platform (API 1.6.0, +// TEAMS.md §7.1). +// +// The DEFINITION and the HANDLER are registered together and the handler runs +// HERE, in the website process; the bot pulls the definitions over the internal +// API and owns every Discord-specific concern. That split is forced — the bot +// container has no `modules` volume, so a module physically cannot put a handler +// in it (§0.4) — and it is also the boundary we would pick anyway: a module +// calling `interaction.deferReply()` would be a module holding a Discord handle. +const slashCommands = new Map() + let coreRegistered = false // Stream ids that predate the module system and may not carry their owner's @@ -214,6 +226,23 @@ const registeredTeamProvider = () => teamProvider /** Is there a Team provider at all? Read by the reconciler and the read API. */ const hasTeamProvider = () => teamProvider !== null +// ── Slash commands (TEAMS.md §7.1) ───────────────────────────────────────── + +/** + * Every registered command WITHOUT its handler — what `/internal/commands` + * serves to the bot. + * + * The handler is stripped rather than merely un-serialisable-and-ignored: this + * is the object that crosses a process boundary, and the definition half is the + * whole of what the bot is allowed to know. `owner` rides along so the bot can + * name the module in a collision warning. + */ +const slashCommandDefinitions = () => + [...slashCommands.values()].map(({ handler, ...definition }) => definition) + +/** One command, handler included. The dispatcher's lookup. */ +const slashCommand = (name) => slashCommands.get(name) || null + // ── Shape checks, run the moment a registrant calls ──────────────────────── // // Split from the collision checks below on the same line PR 3 drew through @@ -315,6 +344,94 @@ function checkPageUrlTemplate(value) { return value } +// A slash command's name and description are validated HERE and not only at the +// bot, for a reason worth stating: the bot registers the whole set in a single +// `REST.put(applicationGuildCommands)`, so ONE malformed definition is rejected +// by Discord as a batch and takes every other command down with it — including +// the bot's own. A definition that cannot be registered must therefore fail at +// `register()`, where it belongs to a module that can be named and marked +// failed, rather than at the next `ready` where it looks like the bot is broken. +// +// **Commands are NOT namespaced under their owner, unlike every other id in this +// file.** Discord's name grammar has no `.` in it, so `uo.guild` is unregistrable +// and the prefix rule cannot be expressed. Collisions are caught by first-come +// instead, with the holder named — and the bot resolves the one collision core +// cannot see (a pulled name against its own built-ins) in the module's disfavour. +const SLASH_NAME = /^[a-z0-9_-]{1,32}$/ +const SLASH_ACCESS = ['everyone', 'linked', 'staff'] + +// §7.1.1: `string | integer | boolean | user`, and deliberately nothing else. No +// subcommand groups, autocomplete, attachments, modals or component +// interactions. Those are exactly the features whose semantics do not survive a +// second platform, and admitting one here is how Discord specifics leak into a +// platform-agnostic registration API by accident. +const SLASH_OPTION_TYPES = ['string', 'integer', 'boolean', 'user'] + +function checkSlashOption(command, option) { + const { name, type, description, required, choices } = option || {} + const where = `registerSlashCommands: ${command}` + if (!SLASH_NAME.test(name || '')) throw new Error(`${where}: bad option name "${name}"`) + if (!SLASH_OPTION_TYPES.includes(type)) { + throw new Error(`${where}: option "${name}" has unsupported type "${type}" (§7.1.1)`) + } + if (!description || description.length > 100) { + throw new Error(`${where}: option "${name}" needs a description of 1-100 characters`) + } + const out = { name, type, description, required: Boolean(required) } + if (choices !== undefined) { + if (!Array.isArray(choices) || !choices.length) { + throw new Error(`${where}: option "${name}" has an empty choices list`) + } + // Only the two option types Discord itself allows choices on. `boolean` is + // already a two-value choice and `user` is a picker; a choices list on + // either is a misunderstanding worth failing rather than dropping. + if (type !== 'string' && type !== 'integer') { + throw new Error(`${where}: option "${name}" is ${type}; choices need string or integer`) + } + out.choices = choices.map((c) => { + if (!c || !c.name || c.value === undefined) { + throw new Error(`${where}: option "${name}" has a choice with no name/value`) + } + return { name: String(c.name), value: c.value } + }) + } + return out +} + +/** + * `registerSlashCommands([{ name, description, options, access, handler }])`. + * + * `access` is enforced TWICE and this copy is not the gate: the bot sets + * Discord-side default member permissions from it where it can, and the + * dispatcher re-checks it on every call. Client-side is about not advertising a + * dead end; the server is the boundary — the same principle the nav follows. + */ +function checkSlashCommandShape(entry) { + const { name, description, options, access, handler } = entry || {} + if (!SLASH_NAME.test(name || '')) { + throw new Error(`registerSlashCommands: bad command name "${name}" (lowercase, 1-32, no dots)`) + } + if (!description || description.length > 100) { + throw new Error(`registerSlashCommands: ${name} needs a description of 1-100 characters`) + } + if (typeof handler !== 'function') throw new Error(`registerSlashCommands: ${name} has no handler()`) + if (access !== undefined && !SLASH_ACCESS.includes(access)) { + throw new Error(`registerSlashCommands: ${name} has unknown access "${access}"`) + } + if (options !== undefined && !Array.isArray(options)) { + throw new Error(`registerSlashCommands: ${name} options must be an array`) + } + const checked = (options || []).map((o) => checkSlashOption(name, o)) + // Discord rejects a definition that puts an optional option before a required + // one, and does it for the whole batch. Sorting silently would change what the + // module wrote; this is the module's own ordering bug and it gets its name. + const firstOptional = checked.findIndex((o) => !o.required) + if (firstOptional !== -1 && checked.slice(firstOptional).some((o) => o.required)) { + throw new Error(`registerSlashCommands: ${name} lists a required option after an optional one`) + } + return { name, description, options: checked, access: access || 'everyone', handler } +} + /** * `registerPostHook({ onSaved, onDeleted })` — both optional, at least one * required. A registration with neither is a subscription that can never fire, @@ -355,7 +472,9 @@ function checkExtensionShape(slot, router, specFile) { * `allStreams()` / `announceLeg()` / the slot routers until `apply()`. */ function stage(owner) { - const staged = { owner, streams: [], legs: [], extensions: [], postHooks: [], teamProviders: [] } + const staged = { + owner, streams: [], legs: [], extensions: [], postHooks: [], teamProviders: [], slashCommands: [], + } return { staged, registerNotificationStreams(entries) { @@ -374,6 +493,10 @@ function stage(owner) { registerTeamProvider(entry) { staged.teamProviders.push(checkTeamProviderShape(entry)) }, + registerSlashCommands(entries) { + if (!Array.isArray(entries)) throw new Error('registerSlashCommands: expected an array') + for (const e of entries) staged.slashCommands.push(checkSlashCommandShape(e)) + }, } } @@ -393,6 +516,7 @@ function apply({ extensions: newExtensions, postHooks: newPostHooks = [], teamProviders: newTeamProviders = [], + slashCommands: newSlashCommands = [], }) { // ── validate ── const seenStreams = new Set() @@ -437,6 +561,14 @@ function apply({ throw new Error(`a team provider is already registered by "${teamProvider.owner}"`) } + const seenCommands = new Set() + for (const c of newSlashCommands) { + const held = slashCommands.get(c.name) + if (held) throw new Error(`slash command "/${c.name}" is already registered by "${held.owner}"`) + if (seenCommands.has(c.name)) throw new Error(`slash command "/${c.name}" registered twice`) + seenCommands.add(c.name) + } + // ── commit — nothing below can fail ── for (const s of newStreams) { streamOwners.set(s.id, owner) @@ -451,6 +583,7 @@ function apply({ } for (const h of newPostHooks) postHooks.set(owner, h) for (const p of newTeamProviders) teamProvider = { owner, ...p } + for (const c of newSlashCommands) slashCommands.set(c.name, { owner, ...c }) } // ── Core's own registrations ─────────────────────────────────────────────── @@ -517,6 +650,7 @@ function _reset() { legs.clear() postHooks.clear() teamProvider = null + slashCommands.clear() coreRegistered = false } @@ -536,6 +670,8 @@ module.exports = { dispatchPostHook, registeredTeamProvider, hasTeamProvider, + slashCommandDefinitions, + slashCommand, stage, apply, registerCore, diff --git a/server/src/modules/version.js b/server/src/modules/version.js index b7ef857..4ff8c60 100644 --- a/server/src/modules/version.js +++ b/server/src/modules/version.js @@ -16,13 +16,12 @@ // client slots `team.overview` / `team.member.row`. module-uo's `coreApi: // "^1.3.0"` still resolves. // -// **The number covers the whole surface; the members arrive by phase.** The three -// this phase implements are live. `activity.push` lands with the Team activity -// feed (§4, phase 3) and `registerSlashCommands` with the Discord commands (§7.1, -// phase 7) — until then each is present and THROWS rather than being absent or, -// worse, silently accepting data into a table that does not exist. MODULE_API.md -// names the phase against each member, so a module author reads what is callable -// today rather than discovering it at runtime. +// **The number covers the whole surface; the members arrived by phase, and all of +// them have now arrived.** `activity.push` landed with the Team activity feed +// (§4, phase 3) and `registerSlashCommands` with the Discord commands (§7.1, +// phase 7); until each did, it was present and THREW rather than being absent or, +// worse, silently accepting data into a table that did not exist. Nothing in +// 1.6.0 throws any more. // // 1.5.0 — a CLIENT addition: `PublicLayout` takes an optional `shell` prop that // renders the page body wrapper core's own pages write by hand (MODULE_API.md diff --git a/server/src/router/v1/internal/internal.controller.js b/server/src/router/v1/internal/internal.controller.js index 4ae78ab..ddaf7af 100644 --- a/server/src/router/v1/internal/internal.controller.js +++ b/server/src/router/v1/internal/internal.controller.js @@ -1,4 +1,5 @@ const botConfig = require('../../../model/botConfig/botConfig.model') +const slashCommands = require('../../../utils/slashCommands') const log = require('../../../utils/logger')('internal') // GET /internal/bot-config — called by the bot process on its own boot so a @@ -16,4 +17,40 @@ async function getBotConfig(req, res) { } } -module.exports = { getBotConfig } +// GET /internal/commands — the registered slash-command definitions, pulled by +// the bot on `ready` and again whenever it is nudged (TEAMS.md §7.1). +// +// `version` is `modules.version()`, the counter every module state change bumps. +// The bot holds the value it registered with and re-PUTs only when it differs, +// which is what makes DEREGISTRATION free: the bot's single whole-set +// `REST.put(applicationGuildCommands)` means a module that is gone is simply +// absent from the next pull, with nobody having to remember to unregister it. +function listCommands(req, res) { + return res.json(slashCommands.definitions()) +} + +// POST /internal/commands/dispatch — run one command and answer with the +// envelope. Never 500s on a handler's behalf: `dispatch` catches per handler and +// reports `{ ok: false, reason }`, so the bot always has something to render and +// a module's failure is its own. +async function dispatchCommand(req, res) { + const { command, options, platform, platformUserId, guildId } = req.body || {} + if (!command) return res.status(400).json({ ok: false, reason: 'unknown' }) + try { + const result = await slashCommands.dispatch({ + command, + options: options && typeof options === 'object' ? options : {}, + platform: platform || 'discord', + platformUserId, + guildId, + }) + return res.json(result) + } catch (err) { + // dispatch() is documented never to throw; if it ever does, that is core's + // bug and not the module's, and it is logged as one. + log.error('internal.dispatchCommand', err) + return res.status(500).json({ ok: false, reason: 'error' }) + } +} + +module.exports = { getBotConfig, listCommands, dispatchCommand } diff --git a/server/src/router/v1/internal/internal.routes.js b/server/src/router/v1/internal/internal.routes.js index 9fa4cfc..5d6af14 100644 --- a/server/src/router/v1/internal/internal.routes.js +++ b/server/src/router/v1/internal/internal.routes.js @@ -16,4 +16,20 @@ router.get( ctrl.getBotConfig, ) +// The slash-command seam (TEAMS.md §7.1). Both stay off the public API and out +// of the OpenAPI document for the same reason /bot-config does: the caller is +// the bot process on the private compose network, and `/internal/*` is not a +// published contract. +router.get( + '/commands', + // #swagger.ignore = true + ctrl.listCommands, +) + +router.post( + '/commands/dispatch', + // #swagger.ignore = true + ctrl.dispatchCommand, +) + module.exports = router diff --git a/server/src/utils/botInternalClient.js b/server/src/utils/botInternalClient.js index 2676174..63967fc 100644 --- a/server/src/utils/botInternalClient.js +++ b/server/src/utils/botInternalClient.js @@ -72,4 +72,16 @@ function reverseModAction({ discordUserId, actionType, appealId }) { }) } -module.exports = { pushConfig, getStatus, announce, reverseModAction } +// Site -> bot: the registered slash-command set has moved, re-pull it +// (TEAMS.md §7.1). Best-effort like everything else here — a bot that is down +// re-pulls on its next `ready` anyway, so a missed nudge costs nothing but the +// delay until the bot reconnects. +// +// Its own endpoint rather than a field on pushConfig, whose body carries the +// DECRYPTED bot token: telling the bot that a module changed should not require +// reading a secret out of the database. +function refreshCommands() { + return call('/internal/refresh-commands', { method: 'POST', body: {} }) +} + +module.exports = { pushConfig, getStatus, announce, reverseModAction, refreshCommands } diff --git a/server/src/utils/slashCommands.js b/server/src/utils/slashCommands.js new file mode 100644 index 0000000..04d5094 --- /dev/null +++ b/server/src/utils/slashCommands.js @@ -0,0 +1,239 @@ +// Chat-platform slash commands: the actor resolver, the access gate, and the +// dispatcher that calls a registrant's handler (TEAMS.md §7.1, API 1.6.0). +// +// Where this sits. The bot owns every Discord-specific concern — deferral, the +// 3-second ack, ephemerality, follow-ups, interaction tokens, embeds — and this +// file owns everything that is not Discord-shaped: who is asking, whether they +// may, and what the answer is. Nothing below imports discord.js or knows what an +// interaction is, which is the whole point: the second platform reuses all of it. +// +// The handler runs HERE rather than in the bot because the bot container has no +// `modules` volume and physically cannot load module code (§0.4). +const authProviders = require('../model/authProviders/authProviders.model') +const userIdentities = require('../model/userIdentities/userIdentities.model') +const users = require('../model/users/users.model') +const modules = require('../modules/loader') +const registries = require('../modules/registries') +const log = require('./logger')('slash-commands') + +// The same two roles every other Team surface calls staff (teamGrants.STAFF_ROLES). +// Required here rather than duplicated, so a change to what "staff" means reaches +// the Discord surface without anyone having to remember this file exists. +const { STAFF_ROLES } = require('../model/teams/teamGrants.model') + +// The handler's own budget, deliberately UNDER the bot's 4s dispatch timeout. +// +// If the bot's abort fires first, the app is left running a handler whose answer +// nobody will read, and the bot reports the same "that failed" either way. Losing +// the race on purpose means the wedged handler is identified HERE, in a log line +// that names the module, and the request ends. +const HANDLER_TIMEOUT_MS = 3000 + +// Discord's hard limits on what can be rendered. Enforced on the way OUT because +// an oversized reply fails inside the bot's `editReply`, where the module that +// produced it cannot be seen — the caller would get "that command failed" for a +// command whose handler worked perfectly. Truncation is silent to the user and +// logged for the operator. +const MAX_TEXT = 2000 +const MAX_FIELDS = 25 +const MAX_FIELD_NAME = 256 +const MAX_FIELD_VALUE = 1024 +const MAX_URL = 512 + +const clamp = (value, max) => (String(value).length > max ? `${String(value).slice(0, max - 1)}…` : String(value)) + +/** + * Is this registrant's command answerable right now? + * + * **The registries have no removal path.** Registration happens once, during + * `load()`, and nothing takes a claim back — a module the operator disables at + * runtime keeps its streams and its announce legs, and would keep its commands + * too. That is tolerable for a stream (a catalog entry nobody publishes to) and + * NOT tolerable for a command, whose handler is live code an operator believes + * they just switched off. + * + * So liveness is asked at the two moments it matters — the pull and the + * dispatch — and it is asked HERE, once, so the two can never disagree. §7.1's + * "deregistration on module unload is free" holds across the restart an + * uninstall asks for; this is the same promise kept for the runtime toggle, + * which that paragraph did not consider. + * + * `core` is not a module and has no record; it is live whenever the process is. + */ +function ownerIsLive(owner) { + if (owner === 'core') return true + if (!modules.isLoaded()) return false + const record = modules.list().find((m) => m.id === owner) + return Boolean(record && record.state === 'started') +} + +/** + * What the bot pulls: the live definitions, and the counter it re-registers on. + * + * Before `load()` has run there is nothing registered and nothing to say, which + * is a legitimate answer rather than an error — a bot that connects during boot + * pulls an empty set and is nudged once the modules are up. Throwing here would + * look to the bot exactly like the app being broken. + */ +function definitions() { + if (!modules.isLoaded()) return { version: 0, commands: [] } + return { + version: modules.version(), + commands: registries.slashCommandDefinitions().filter((c) => ownerIsLive(c.owner)), + } +} + +/** + * Who is running this command, in platform-neutral terms. + * + * A handler never parses a platform payload and never learns anything + * platform-shaped beyond `platform` itself. Everything here is resolved before + * the handler is entered, so a module cannot decide for itself who it is talking + * to — the actor is core's answer, not the caller's claim. + * + * **The Discord provider is found by `kind`, not by id.** `auth_providers.id` is + * an operator-chosen slug and only the CONVENTIONAL deployment calls it + * "discord"; the kind column is the enum. Resolving by id would silently return + * "not linked" for every user on a deployment that named its provider anything + * else, which reads as a bug in linking rather than a bug here. + * + * A non-active account resolves to UNLINKED rather than to itself: a banned or + * disabled user keeping `access: 'linked'` commands would make Discord the one + * surface a ban does not reach. + */ +async function resolveActor({ platform, platformUserId, guildId = null }) { + const actor = { + platform, + platformUserId: platformUserId ? String(platformUserId) : null, + guildId: guildId || null, + userId: null, + role: null, + isLinked: false, + isStaff: false, + } + if (platform !== 'discord' || !actor.platformUserId) return actor + + const providers = (await authProviders.list()).filter((p) => p.kind === 'discord') + for (const provider of providers) { + // eslint-disable-next-line no-await-in-loop + const identity = await userIdentities.findByProviderSubject(provider.id, actor.platformUserId) + if (!identity) continue + // eslint-disable-next-line no-await-in-loop + const user = await users.getById(identity.user_id) + if (!user || user.status !== 'active') continue + actor.userId = user.id + // `role` alongside `isStaff`, because the two answer different questions and + // collapsing them loses one. `isStaff` is core's gate for `access: 'staff'`; + // `role` is what a module needs to place the caller on its OWN ladder — a + // module with audience rungs distinguishes admin from moderator and cannot + // from a boolean. It is the same pair `projectRoster`'s viewer already + // carries (§3.3), not a new class of disclosure. + actor.role = user.role + actor.isLinked = true + actor.isStaff = STAFF_ROLES.includes(user.role) + break + } + return actor +} + +/** Does this actor clear the command's declared access level? */ +function permitted(access, actor) { + if (access === 'staff') return actor.isStaff + if (access === 'linked') return actor.isLinked + return true +} + +/** + * Shape and clamp whatever a handler returned into the response envelope. + * + * A handler returning nothing at all is a handler that ran and had nothing to + * say, which is not an error — the bot renders it as an empty acknowledgement. + */ +function envelope(result, command) { + const out = {} + const value = result || {} + if (value.text) out.text = clamp(value.text, MAX_TEXT) + if (value.title) out.title = clamp(value.title, MAX_FIELD_NAME) + if (Array.isArray(value.fields) && value.fields.length) { + if (value.fields.length > MAX_FIELDS) { + log.warn('handler returned too many fields; truncated', { command, fields: value.fields.length }) + } + out.fields = value.fields.slice(0, MAX_FIELDS).map((f) => ({ + name: clamp(f && f.name ? f.name : '—', MAX_FIELD_NAME), + value: clamp(f && f.value ? f.value : '—', MAX_FIELD_VALUE), + inline: Boolean(f && f.inline), + })) + } + // Absolute http(s) only. A handler builds its own link from `ctx.site.baseUrl` + // — core cannot, since Teams have no core page to link to (§3.1 as amended) — + // so the scheme check is the whole of what is enforced here: anything else, + // `javascript:` above all, is dropped rather than posted into a channel the + // operator's members trust. + if (value.url && /^https?:\/\//i.test(value.url)) out.url = clamp(value.url, MAX_URL) + out.ephemeral = Boolean(value.ephemeral) + // A private aside to the caller, delivered ALONGSIDE a public answer — §9 + // answer 5's "the public projection plus an ephemeral prompt to link". One + // reply cannot be both public and ephemeral, so this is a second message, and + // that it is a second message is the platform's business rather than the + // handler's: `notice` says "say this to the caller only" and the bot decides it + // is a follow-up. + if (value.notice) out.notice = clamp(value.notice, MAX_TEXT) + return out +} + +/** + * Run one command and answer with `{ ok, ... }`. + * + * **Never throws, and never returns a handler's own `ok`.** The result is nested + * under `response` precisely so a module cannot forge the success flag the bot + * branches on — the envelope is data the handler produced, `ok` is core's verdict + * on whether it produced it. + * + * A refusal carries a `reason` and no user-facing copy: the phrasing of "you need + * to link your account" is a platform's own business, and putting the sentence + * here would be core writing Discord's voice. + * + * Failure isolation is per handler, per call. A module whose handler throws or + * wedges costs its own command and nothing else — and cannot cost the bot + * anything at all, because the handler does not run there. + */ +async function dispatch({ command, options = {}, platform = 'discord', platformUserId = null, guildId = null }) { + const entry = registries.slashCommand(command) + // A disabled module's command is UNKNOWN, not forbidden: Discord may still be + // advertising it — the whole-set PUT that removes it has not necessarily + // happened yet — and "there is no such command" is the truthful answer for one + // whose owner is switched off. + if (!entry || !ownerIsLive(entry.owner)) return { ok: false, reason: 'unknown' } + + let actor + try { + actor = await resolveActor({ platform, platformUserId, guildId }) + } catch (err) { + // Identity resolution is core's, not the module's — a database hiccup here + // is not the command failing, and saying so would send an operator to read + // module code that never ran. + log.error('actor resolution failed', { command, message: err.message }) + return { ok: false, reason: 'error' } + } + + // The gate. The bot also sets Discord-side default member permissions from + // `access` where it can, but that is advertising; this is the boundary. + if (!permitted(entry.access, actor)) { + return { ok: false, reason: 'forbidden', access: entry.access, isLinked: actor.isLinked } + } + + try { + const result = await Promise.race([ + entry.handler({ command, options, actor }), + new Promise((_, reject) => { + setTimeout(() => reject(new Error('handler timed out')), HANDLER_TIMEOUT_MS).unref() + }), + ]) + return { ok: true, response: envelope(result, command) } + } catch (err) { + log.error('slash command handler failed', { command, owner: entry.owner, message: err.message }) + return { ok: false, reason: 'error' } + } +} + +module.exports = { definitions, resolveActor, dispatch, envelope, ownerIsLive, HANDLER_TIMEOUT_MS } diff --git a/server/test/slashCommands.test.js b/server/test/slashCommands.test.js new file mode 100644 index 0000000..66c421a --- /dev/null +++ b/server/test/slashCommands.test.js @@ -0,0 +1,261 @@ +// ── Slash commands: registration, the actor, and the dispatcher ──────────── +// +// TEAMS.md §7.1, phase 7. Three things are worth a test here and none of them +// can be exercised by hand without a Discord guild: +// +// 1. **Registration rejects a definition Discord would reject as a batch.** The +// bot registers the whole set in one PUT, so one bad option type costs every +// command — including the bot's own. That has to fail at `register()`. +// 2. **The dispatcher is the access boundary**, not the Discord-side permission +// default, which cannot express "has a linked account" at all. +// 3. **A handler's failure is its own.** A throw, a hang, and a disabled owner +// each produce an answer the bot can render, and never an exception. +process.env.DB_HOST = '127.0.0.1' +process.env.DB_PORT = '59999' + +const { test, beforeEach, after } = require('node:test') +const assert = require('node:assert/strict') + +const registries = require('../src/modules/registries') +const slash = require('../src/utils/slashCommands') +const loader = require('../src/modules/loader') +const authProviders = require('../src/model/authProviders/authProviders.model') +const userIdentities = require('../src/model/userIdentities/userIdentities.model') +const users = require('../src/model/users/users.model') +const db = require('../src/utils/db') + +after(() => db.close()) + +const originals = { + isLoaded: loader.isLoaded, + list: loader.list, + version: loader.version, + providerList: authProviders.list, + findIdentity: userIdentities.findByProviderSubject, + getUser: users.getById, +} + +// The loader is stubbed rather than driven: `load()` needs a modules directory +// on disk, and every property below is about what the registry and the +// dispatcher do with a state, not about how the state was reached. +function loaded(state = 'started', id = 'uo') { + loader.isLoaded = () => true + loader.list = () => [{ id, state }] + loader.version = () => 42 +} + +beforeEach(() => { + registries._reset() + Object.assign(loader, { isLoaded: originals.isLoaded, list: originals.list, version: originals.version }) + Object.assign(authProviders, { list: originals.providerList }) + Object.assign(userIdentities, { findByProviderSubject: originals.findIdentity }) + Object.assign(users, { getById: originals.getUser }) +}) + +const cmd = (over = {}) => ({ + name: 'guild', + description: 'Show a guild', + handler: async () => ({ text: 'ok' }), + ...over, +}) + +/** Register a batch as `owner`; returns the error message, or null on success. */ +function tryRegister(owner, commands) { + const api = registries.stage(owner) + try { + api.registerSlashCommands(commands) + registries.apply(api.staged) + return null + } catch (err) { + return err.message + } +} + +// ── Registration ─────────────────────────────────────────────────────────── + +test('a registered command is served without its handler', () => { + assert.equal(tryRegister('uo', [cmd()]), null) + const [definition] = registries.slashCommandDefinitions() + assert.equal(definition.name, 'guild') + assert.equal(definition.owner, 'uo') + assert.equal(definition.access, 'everyone', 'access defaults rather than being undefined on the wire') + assert.equal(definition.handler, undefined, 'the handler never crosses the process boundary') + assert.equal(typeof registries.slashCommand('guild').handler, 'function') +}) + +// Discord's name grammar has no dot in it, so the owner-prefix rule every other +// id in registries.js follows cannot be expressed here. Collisions are the +// substitute, and they have to name the holder. +test('two registrants cannot hold the same command name', () => { + tryRegister('uo', [cmd()]) + assert.match(tryRegister('rust', [cmd()]), /already registered by "uo"/) + // The same name twice inside ONE batch, which the held-by check above cannot + // catch — nothing is committed yet when the second entry is validated. + registries._reset() + assert.match(tryRegister('uo', [cmd(), cmd()]), /registered twice/) +}) + +test('a name Discord would reject is refused at registration', () => { + assert.match(tryRegister('uo', [cmd({ name: 'uo.guild' })]), /bad command name/) + assert.match(tryRegister('uo', [cmd({ name: 'Guild' })]), /bad command name/) + assert.match(tryRegister('uo', [cmd({ name: 'g'.repeat(33) })]), /bad command name/) + assert.match(tryRegister('uo', [cmd({ description: '' })]), /description of 1-100/) + assert.match(tryRegister('uo', [cmd({ handler: 'nope' })]), /has no handler/) + assert.match(tryRegister('uo', [cmd({ access: 'members' })]), /unknown access "members"/) +}) + +// §7.1.1 keeps the option schema small on purpose: subcommand groups, +// autocomplete, attachments and modals are the features whose semantics do not +// survive a second platform. +test('only the four option types survive registration', () => { + const opt = (over) => cmd({ options: [{ name: 'x', type: 'string', description: 'd', ...over }] }) + assert.equal(tryRegister('uo', [opt({})]), null) + registries._reset() + assert.match(tryRegister('uo', [opt({ type: 'attachment' })]), /unsupported type "attachment"/) + assert.match(tryRegister('uo', [opt({ type: 'boolean', choices: [{ name: 'a', value: 1 }] })]), /choices need string or integer/) + assert.match(tryRegister('uo', [opt({ choices: [] })]), /empty choices list/) +}) + +// Discord rejects the whole batch for this, so it cannot be left to be +// discovered at the next `ready`. +test('a required option after an optional one is refused', () => { + const options = [ + { name: 'a', type: 'string', description: 'd', required: false }, + { name: 'b', type: 'string', description: 'd', required: true }, + ] + assert.match(tryRegister('uo', [cmd({ options })]), /required option after an optional one/) +}) + +// The validate-then-commit rule the rest of registries.js follows: a batch that +// fails leaves nothing behind, or the bot would pull a half-registered set. +test('a batch that fails registers none of it', () => { + tryRegister('uo', [cmd(), cmd({ name: 'bad name' })]) + assert.deepEqual(registries.slashCommandDefinitions(), []) +}) + +// ── Liveness ─────────────────────────────────────────────────────────────── + +// The registries have no removal path — nothing takes a registration back — so a +// module an operator disables at runtime would otherwise keep answering. +test('a disabled module’s command disappears from the pull and stops dispatching', async () => { + tryRegister('uo', [cmd()]) + loaded('started') + assert.equal(slash.definitions().commands.length, 1) + assert.equal(slash.definitions().version, 42) + + loaded('disabled') + assert.deepEqual(slash.definitions().commands, []) + assert.deepEqual(await slash.dispatch({ command: 'guild' }), { ok: false, reason: 'unknown' }) +}) + +test('before load() there is nothing registered and that is an answer, not an error', () => { + loader.isLoaded = () => false + assert.deepEqual(slash.definitions(), { version: 0, commands: [] }) +}) + +// ── The actor ────────────────────────────────────────────────────────────── + +function identity({ providerId = 'discord', kind = 'discord', role = 'player', status = 'active' } = {}) { + authProviders.list = async () => [{ id: providerId, kind }] + userIdentities.findByProviderSubject = async (provider, subject) => + (provider === providerId && subject === '555' ? { user_id: 9 } : null) + users.getById = async (id) => (id === 9 ? { id: 9, role, status } : null) +} + +// `auth_providers.id` is an operator-chosen slug; `kind` is the enum. Resolving +// by id would report "not linked" for every user on a deployment that named its +// Discord provider anything else. +test('the Discord provider is found by kind, whatever the operator named it', async () => { + identity({ providerId: 'our-discord' }) + const actor = await slash.resolveActor({ platform: 'discord', platformUserId: '555' }) + assert.deepEqual(actor, { + platform: 'discord', platformUserId: '555', guildId: null, userId: 9, role: 'player', isLinked: true, isStaff: false, + }) +}) + +test('a non-Discord kind on a provider named "discord" does not link anyone', async () => { + identity({ kind: 'oidc' }) + const actor = await slash.resolveActor({ platform: 'discord', platformUserId: '555' }) + assert.equal(actor.isLinked, false) +}) + +// Otherwise Discord would be the one surface a ban does not reach. +test('a banned or disabled account resolves as unlinked', async () => { + identity({ status: 'banned' }) + const actor = await slash.resolveActor({ platform: 'discord', platformUserId: '555' }) + assert.equal(actor.isLinked, false) + assert.equal(actor.userId, null) +}) + +test('staff is the same two roles every other Team surface means by it', async () => { + identity({ role: 'moderator' }) + assert.equal((await slash.resolveActor({ platform: 'discord', platformUserId: '555' })).isStaff, true) + identity({ role: 'editor' }) + assert.equal((await slash.resolveActor({ platform: 'discord', platformUserId: '555' })).isStaff, false) +}) + +// ── Dispatch ─────────────────────────────────────────────────────────────── + +test('access is enforced by the dispatcher, not by Discord’s permission model', async () => { + identity({ role: 'player' }) + loaded() + tryRegister('uo', [cmd({ name: 'linked-only', access: 'linked' }), cmd({ name: 'staff-only', access: 'staff' })]) + + const unlinked = { command: 'linked-only', platformUserId: 'nobody' } + assert.deepEqual(await slash.dispatch(unlinked), { + ok: false, reason: 'forbidden', access: 'linked', isLinked: false, + }) + // A linked player clears `linked` and not `staff` — the distinction Discord's + // default_member_permissions cannot express at all. + assert.equal((await slash.dispatch({ command: 'linked-only', platformUserId: '555' })).ok, true) + assert.equal((await slash.dispatch({ command: 'staff-only', platformUserId: '555' })).reason, 'forbidden') +}) + +test('a handler that throws costs its own command and answers renderably', async () => { + loaded() + tryRegister('uo', [cmd({ handler: async () => { throw new Error('boom') } })]) + assert.deepEqual(await slash.dispatch({ command: 'guild' }), { ok: false, reason: 'error' }) +}) + +test('a handler that hangs is bounded, not waited on', async () => { + loaded() + tryRegister('uo', [cmd({ handler: () => new Promise(() => {}) })]) + const started = Date.now() + const res = await slash.dispatch({ command: 'guild' }) + assert.deepEqual(res, { ok: false, reason: 'error' }) + assert.ok(Date.now() - started < slash.HANDLER_TIMEOUT_MS + 500) +}) + +// `ok` is core's verdict on whether the handler produced an answer, so it lives +// outside the envelope where a handler cannot write it. +test('a handler cannot forge the success flag', async () => { + loaded() + tryRegister('uo', [cmd({ handler: async () => ({ ok: false, text: 'hi' }) })]) + const res = await slash.dispatch({ command: 'guild' }) + assert.equal(res.ok, true) + assert.equal(res.response.ok, undefined) + assert.equal(res.response.text, 'hi') +}) + +// An oversized reply fails inside the bot's editReply, where the module that +// produced it cannot be seen — the caller would get "that command failed" for a +// handler that worked. +test('the envelope is clamped to what Discord will render', () => { + const out = slash.envelope({ + text: 'x'.repeat(3000), + fields: Array.from({ length: 30 }, (_, i) => ({ name: `n${i}`, value: 'v'.repeat(2000) })), + url: 'javascript:alert(1)', + notice: 'link your account', + }, 'guild') + assert.equal(out.text.length, 2000) + assert.equal(out.fields.length, 25) + assert.equal(out.fields[0].value.length, 1024) + assert.equal(out.url, undefined, 'only absolute http(s) survives') + assert.equal(out.notice, 'link your account') +}) + +test('a handler that returns nothing has simply said nothing', async () => { + loaded() + tryRegister('uo', [cmd({ handler: async () => undefined })]) + assert.deepEqual(await slash.dispatch({ command: 'guild' }), { ok: true, response: { ephemeral: false } }) +})