From cecd72915f089176a5f385491a352330693c0ed3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 18:53:34 -0500 Subject: [PATCH 1/2] feat(teams): the slash-command seam, and the first command through it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 7 of TEAMS.md. `api.registerSlashCommands` stops throwing: a module registers a command's DEFINITION and its HANDLER together, the bot pulls the definitions over the internal listener and runs none of our code, and the handler executes here — forced by the bot container having no `modules` volume, and the right boundary anyway. Registration validates what Discord would reject as a batch (names, description lengths, the four option types, required-before-optional), because the bot registers the whole set in one PUT and a single bad entry costs every command including the bot's own. Commands are not namespaced under their owner — there is no dot in Discord's name grammar — so collisions are first-come with the holder named. The dispatcher is the access boundary: `linked` has no Discord equivalent, so the platform-side permission default can only ever be advertising. It resolves the actor by `auth_providers.kind` rather than the id slug, treats a banned account as unlinked, bounds a handler under the bot's own timeout, and keeps `ok` outside the envelope so a handler cannot forge it. Liveness is asked at both the pull and the dispatch. The registries have no removal path, so a module an operator disables at runtime would otherwise keep a live handler behind a command Discord still advertises. Co-Authored-By: Claude --- .gitea/workflows/pr-checks.yml | 14 +- bot/package.json | 1 + bot/src/discord/discordManager.js | 57 +++- bot/src/discord/dynamicCommands.js | 210 ++++++++++++++ bot/src/internal/internal.controller.js | 24 +- bot/src/internal/internal.routes.js | 1 + bot/src/site/appInternalClient.js | 76 +++++ bot/test/appInternalClient.test.js | 77 ++++++ bot/test/dynamicCommands.test.js | 225 +++++++++++++++ server/routes.guards.json | 16 ++ server/routes.manifest.json | 8 + server/src/model/teams/teamGrants.model.js | 4 + server/src/modules/lifecycle.js | 36 +++ server/src/modules/loader.js | 17 +- server/src/modules/registries.js | 138 ++++++++- server/src/modules/version.js | 13 +- .../router/v1/internal/internal.controller.js | 39 ++- .../src/router/v1/internal/internal.routes.js | 16 ++ server/src/utils/botInternalClient.js | 14 +- server/src/utils/slashCommands.js | 239 ++++++++++++++++ server/test/slashCommands.test.js | 261 ++++++++++++++++++ 21 files changed, 1459 insertions(+), 27 deletions(-) create mode 100644 bot/src/discord/dynamicCommands.js create mode 100644 bot/src/site/appInternalClient.js create mode 100644 bot/test/appInternalClient.test.js create mode 100644 bot/test/dynamicCommands.test.js create mode 100644 server/src/utils/slashCommands.js create mode 100644 server/test/slashCommands.test.js 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..3587a73 --- /dev/null +++ b/bot/src/discord/dynamicCommands.js @@ -0,0 +1,210 @@ +// 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] } +} + +/** + * 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 has to be decided BEFORE the answer exists, because it is a + // property of the deferral. A command declared for linked users only is + // answered privately by default — its answer is about the caller's own + // account — and everything else defers publicly; a handler that wants the + // opposite says so, and the follow-up carries 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. + if (!res.ok) { + log.warn('command dispatch failed', { command: definition.name, error: res.error }) + await interaction.editReply({ content: refusal({ reason: 'error' }) }) + return true + } + if (!res.data || !res.data.ok) { + await interaction.editReply({ content: refusal(res.data || {}) }) + return true + } + + const envelope = res.data.response || {} + await interaction.editReply(render(envelope)) + + // 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..58a0232 --- /dev/null +++ b/bot/test/dynamicCommands.test.js @@ -0,0 +1,225 @@ +// ── 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]), + 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. +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/) + assert.equal(interaction.calls.filter(([kind]) => kind === 'edit').length, 1) +}) + +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/) +}) + +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 } }) +}) -- 2.49.1 From aca4d231796b0437d1d320c72806a1f02c00a9ca Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 19:08:12 -0500 Subject: [PATCH 2/2] fix(teams): a private answer has to be private, and the deferral decides that MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found on the live rig. Ephemerality is a property of the DEFERRAL, which happens before the handler has said anything — so the envelope's `ephemeral` was being read and then ignored, and `/guild`'s "not shown to your account" refusal was posted into the channel, announcing a member's access level to everyone in it. When the handler wants privacy the deferral did not give it, the deferred reply is now withdrawn and the answer arrives as an ephemeral follow-up. The interaction token stays valid, so this is a supported path and not a trick; the cost is a "thinking..." that appears and vanishes. There is no reverse case — a command deferred privately must not become public because a handler omitted a flag — and a refusal is always private whatever the command's usual privacy. Co-Authored-By: Claude --- bot/src/discord/dynamicCommands.js | 48 +++++++++++++++++++++++++----- bot/test/dynamicCommands.test.js | 46 +++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 9 deletions(-) diff --git a/bot/src/discord/dynamicCommands.js b/bot/src/discord/dynamicCommands.js index 3587a73..3632df1 100644 --- a/bot/src/discord/dynamicCommands.js +++ b/bot/src/discord/dynamicCommands.js @@ -145,6 +145,30 @@ function render(envelope) { 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. * @@ -160,11 +184,17 @@ async function execute(interaction) { const definition = pulled.find((c) => c.name === interaction.commandName) if (!definition) return false - // Ephemerality has to be decided BEFORE the answer exists, because it is a - // property of the deferral. A command declared for linked users only is - // answered privately by default — its answer is about the caller's own - // account — and everything else defers publicly; a handler that wants the - // opposite says so, and the follow-up carries it. + // **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 }) @@ -178,18 +208,20 @@ async function execute(interaction) { // 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 interaction.editReply({ content: refusal({ reason: 'error' }) }) + await reply(interaction, { text: refusal({ reason: 'error' }), ephemeral: true }, ephemeral) return true } if (!res.data || !res.data.ok) { - await interaction.editReply({ content: refusal(res.data || {}) }) + await reply(interaction, { text: refusal(res.data || {}), ephemeral: true }, ephemeral) return true } const envelope = res.data.response || {} - await interaction.editReply(render(envelope)) + 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 diff --git a/bot/test/dynamicCommands.test.js b/bot/test/dynamicCommands.test.js index 58a0232..a7882b4 100644 --- a/bot/test/dynamicCommands.test.js +++ b/bot/test/dynamicCommands.test.js @@ -53,6 +53,7 @@ function fakeInteraction({ commandName = 'guild', options = {}, userId = '555' } }, 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]), } } @@ -195,6 +196,46 @@ test('a notice is not repeated when the answer was already private', async () => // 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() @@ -204,7 +245,9 @@ test('a refusal is phrased by the bot and edited into the deferred reply', async const interaction = fakeInteraction() await dynamic.execute(interaction) assert.match(interaction.calls.at(-1)[1].content, /Link your Discord account/) - assert.equal(interaction.calls.filter(([kind]) => kind === 'edit').length, 1) + // 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 () => { @@ -214,6 +257,7 @@ test('an unreachable app is the same sentence to the member and a different line 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 () => { -- 2.49.1