From e111c857c3aefc8997af8d11b1a1d45c8547b242 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 18:08:01 -0800 Subject: [PATCH 1/3] fix(discord): only the installer toggles sync; reads need a connected row; /summary cooldown; mute reads its whole argument (F161) Follow-ups from the #3465 and #3469 reviews: - /discord-enable and /discord-disable use the bridge commands' rule: the caller's Discord id must equal the row's proven discordUserId. A legacy row with no recorded id refuses everyone; adding the bot again records it. - The per-guild register route registers nothing that is already global (an empty bulk PUT also clears old duplicates). - /tldr and /summary act only on a connected row; a settings write on a row that isn't connected writes nothing and says so. /status still reports. - /summary: one run per row per 60 s, claimed before the run. - Boot-time command PUT: 10 s timeout, non-zero exit on failure; start.sh keeps booting. - /mute reads its whole argument on Telegram and Slack ("2 hours" = 120); a unit that doesn't parse gets usage on all three platforms; a bare number is still minutes. - contexts: [0] on the bridge commands; a paused-row test. - Connectors: a Discord row in error gets "Add the bot again" (en, zh-CN). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DPFVAooJnqCSwrdxVw --- .../routes/discord.management-auth.test.js | 2 + .../unit/routes/discord.relayCommands.test.js | 101 +++++++++++++++++ .../routes/slack.webhook.commands.test.js | 20 +++- .../unit/routes/telegram.webhook.test.js | 34 ++++++ .../scripts/deployDiscordCommands.test.js | 47 +++++++- .../unit/services/chatRelayCore.test.js | 14 ++- .../services/discordCommandService.test.js | 45 ++++++++ .../discordMultiCommandService.test.js | 13 +++ .../unit/services/discordService.test.js | 25 ++++ .../config/discordApplicationCommands.json | 17 ++- backend/routes/discord.ts | 12 +- backend/routes/webhooks/slack.ts | 7 +- backend/routes/webhooks/telegram.ts | 7 +- backend/scripts/deploy-discord-commands.js | 24 +++- backend/services/chatRelay/chatRelayCore.ts | 24 ++-- backend/services/discordCommandService.ts | 32 +++++- .../services/discordMultiCommandService.ts | 10 +- .../services/discordRelayCommandService.ts | 107 +++++++++++++----- backend/services/discordService.ts | 51 +++------ docs/discord/DISCORD.md | 17 ++- docs/discord/REGISTER_DISCORD_COMMANDS.md | 16 ++- frontend/src/i18n/locales/en.json | 3 +- frontend/src/i18n/locales/zh-CN.json | 3 +- .../ConnectorsPage.channels.test.tsx | 33 ++++++ frontend/src/v2/connectors/channelModel.ts | 2 +- .../src/v2/connectors/useChannelConnectors.ts | 23 +++- 26 files changed, 582 insertions(+), 107 deletions(-) diff --git a/backend/__tests__/unit/routes/discord.management-auth.test.js b/backend/__tests__/unit/routes/discord.management-auth.test.js index 36cb43a8ea..a5d0845d36 100644 --- a/backend/__tests__/unit/routes/discord.management-auth.test.js +++ b/backend/__tests__/unit/routes/discord.management-auth.test.js @@ -172,6 +172,8 @@ describe('discord management route auth', () => { expect(res.status).toBe(200); expect(DiscordService).toHaveBeenCalledWith('integration-1'); + // F161 item 2: the route no longer claims the legacy set per guild; it is global. + expect(res.body.commands).toEqual([]); }); it('requires auth and admin access for register-all', async () => { diff --git a/backend/__tests__/unit/routes/discord.relayCommands.test.js b/backend/__tests__/unit/routes/discord.relayCommands.test.js index 0b2c03346e..7770fd27a1 100644 --- a/backend/__tests__/unit/routes/discord.relayCommands.test.js +++ b/backend/__tests__/unit/routes/discord.relayCommands.test.js @@ -52,6 +52,7 @@ const Integration = require('../../../models/Integration'); const Summary = require('../../../models/Summary'); const { runCommandForIntegration } = require('../../../services/discordMultiCommandService'); const discordRoutes = require('../../../routes/discord'); +const { resetSummaryCooldowns } = require('../../../services/discordRelayCommandService'); const app = express(); app.use('/api/discord/interactions', express.raw({ type: '*/*' })); @@ -110,6 +111,7 @@ beforeEach(() => { jest.clearAllMocks(); mockRows.length = 0; mockRows.push(row()); + resetSummaryCooldowns(); // The pod's creator: the bind rule /tldr re-checks (F143) passes for them. User.findById.mockResolvedValue({ _id: 'user-owner', role: 'user' }); }); @@ -171,6 +173,14 @@ describe('/mute and /unmute', () => { expect(res.body.data.flags).toBe(64); }); + // F161 item 7: the core's usage outcome, in Discord's words. + test.each(['90s', '1.5h'])('/mute %s answers with usage and writes nothing', async (value) => { + const res = await command('mute', { options: [{ name: 'duration', value }] }); + expect(Integration.findByIdAndUpdate).not.toHaveBeenCalled(); + expect(res.body.data.flags).toBe(64); + expect(res.body.data.content).toContain('Usage: /mute'); + }); + test('/unmute clears relayMutedUntil', async () => { await command('unmute'); expect(Integration.findByIdAndUpdate).toHaveBeenCalledWith('row-1', { $unset: { 'config.relayMutedUntil': '' } }); @@ -286,3 +296,94 @@ test('/help answers anywhere, ephemerally', async () => { expect(res.body.data.content).toContain('/mode'); expect(res.body.data.content).toContain('/tldr'); }); + +const settle = async () => { + await new Promise((resolve) => { setImmediate(resolve); }); + await new Promise((resolve) => { setImmediate(resolve); }); +}; + +// F161 item 3: /summary and /tldr run only on a connected row (F132's relay +// rule); a settings write on a row that is not connected says so. +describe('a bound row that is not connected', () => { + test.each(['disconnected', 'error', 'pending'])('/tldr on a %s row posts nothing', async (status) => { + mockRows[0] = { ...row(), status }; + Summary.findOne.mockReturnValue({ sort: () => Promise.resolve({ title: 'T', content: 'secret pod text' }) }); + const res = await command('tldr'); + expect(res.body.type).toBe(4); + expect(res.body.data.flags).toBe(64); + expect(res.body.data.content).not.toContain('secret pod text'); + expect(res.body.data.content).toContain("isn't active"); + }); + + test('/summary on a disconnected row runs nothing and answers at once', async () => { + mockRows[0] = { ...row({ webhookListenerEnabled: true }), status: 'disconnected' }; + const res = await command('summary'); + expect(res.body.type).toBe(4); + expect(res.body.data.flags).toBe(64); + expect(res.body.data.content).toContain("isn't active"); + await settle(); + expect(runCommandForIntegration).not.toHaveBeenCalled(); + }); + + test.each([ + ['mode', [{ name: 'mode', value: 'mirror' }]], + ['mute', [{ name: 'duration', value: '30m' }]], + ['unmute', undefined], + ])('/%s from the installer on a disconnected row writes nothing and says so', async (name, options) => { + mockRows[0] = { ...row(), status: 'disconnected' }; + const res = await command(name, { options }); + expect(Integration.findByIdAndUpdate).not.toHaveBeenCalled(); + expect(res.body.data.flags).toBe(64); + expect(res.body.data.content).toContain("isn't connected right now"); + }); +}); + +// F161 item 4: each /summary is an LLM run, so one row runs it at most once a minute. +test('/summary twice inside a minute runs once; the second is told to wait, ephemerally', async () => { + mockRows[0] = row({ webhookListenerEnabled: true }); + runCommandForIntegration.mockResolvedValue({ result: { success: true, content: 'ok' } }); + const first = await command('summary', { actor: STRANGER }); + expect(first.body.type).toBe(5); + await settle(); + const second = await command('summary', { actor: INSTALLER }); + expect(second.body.type).toBe(4); + expect(second.body.data.flags).toBe(64); + expect(second.body.data.content).toMatch(/less than a minute ago/); + await settle(); + expect(runCommandForIntegration).toHaveBeenCalledTimes(1); +}); + +test('/summary runs again once the minute has passed', async () => { + mockRows[0] = row({ webhookListenerEnabled: true }); + runCommandForIntegration.mockResolvedValue({ result: { success: true, content: 'ok' } }); + const realNow = Date.now; + try { + await command('summary'); + await settle(); + Date.now = () => realNow() + 61_000; + const again = await command('summary'); + expect(again.body.type).toBe(5); + await settle(); + expect(runCommandForIntegration).toHaveBeenCalledTimes(2); + } finally { + Date.now = realNow; + } +}); + +// F161 item 9: an admin pause takes the row out of every command's reach. +describe('a row under an admin pause', () => { + beforeEach(() => { mockRows[0] = row({ adminPause: { at: new Date(), by: 'admin-1' } }); }); + + test('the installer cannot change its settings', async () => { + const res = await command('mode', { options: [{ name: 'mode', value: 'mirror' }] }); + expect(Integration.findByIdAndUpdate).not.toHaveBeenCalled(); + expect(res.body.data.flags).toBe(64); + }); + + test('/tldr posts nothing', async () => { + Summary.findOne.mockReturnValue({ sort: () => Promise.resolve({ title: 'T', content: 'secret pod text' }) }); + const res = await command('tldr'); + expect(res.body.data.flags).toBe(64); + expect(res.body.data.content).not.toContain('secret pod text'); + }); +}); diff --git a/backend/__tests__/unit/routes/slack.webhook.commands.test.js b/backend/__tests__/unit/routes/slack.webhook.commands.test.js index c45054e29c..719d096cba 100644 --- a/backend/__tests__/unit/routes/slack.webhook.commands.test.js +++ b/backend/__tests__/unit/routes/slack.webhook.commands.test.js @@ -71,9 +71,11 @@ describe('Slack /commonly command replies', () => { ['mute', 60], ['mute 15', 15], ['mute 0', 60], - ['mute -5', 1], ['mute 99999', 24 * 60], - ['mute soon', 60], + ['mute 2h', 120], + // F161: the whole argument reaches the parse, not just its first word. + ['mute 2 hours', 120], + ['mute 30 min', 30], ])('%s mutes for %i minutes', async (text, minutes) => { bound(); const before = Date.now(); @@ -86,6 +88,20 @@ describe('Slack /commonly command replies', () => { expect(until).toBeLessThanOrEqual(Date.now() + minutes * 60000); }); + // F161: a duration the shared core cannot read is usage, not a guess. + test.each(['mute -5', 'mute soon', 'mute 90s'])('%s answers with usage and writes nothing', async (text) => { + bound(); + const res = await command(text); + expect(res.body).toEqual({ response_type: 'ephemeral', text: 'Usage: /commonly mute 30m|2h' }); + expect(Integration.updateOne).not.toHaveBeenCalled(); + }); + + test('mode still reads only its first word', async () => { + bound(); + const res = await command('mode attention please'); + expect(res.body).toEqual({ response_type: 'ephemeral', text: 'Relay mode: attention.' }); + }); + test('unmute clears only the owner mute', async () => { bound(); const res = await command('unmute'); diff --git a/backend/__tests__/unit/routes/telegram.webhook.test.js b/backend/__tests__/unit/routes/telegram.webhook.test.js index 4e9a5d189b..b1db1cc19b 100644 --- a/backend/__tests__/unit/routes/telegram.webhook.test.js +++ b/backend/__tests__/unit/routes/telegram.webhook.test.js @@ -337,6 +337,40 @@ describe('bridge command surface (/mode /status /mute /help)', () => { expect(until).toBeGreaterThan(Date.now()); }); + // F161 item 6: the whole argument reaches the parse, so `2 hours` is 120 minutes. + it('/mute 2 hours mutes for 120 minutes', async () => { + const before = Date.now(); + const res = await post('/mute 2 hours'); + expect(res.status).toBe(200); + const [, update] = Integration.findByIdAndUpdate.mock.calls[0]; + const until = new Date(update.$set['config.relayMutedUntil']).getTime(); + expect(until).toBeGreaterThanOrEqual(before + 120 * 60000); + expect(until).toBeLessThanOrEqual(Date.now() + 120 * 60000); + expect(telegramService.sendMessage).toHaveBeenCalledWith('bot-token', '42', expect.stringContaining('Muted for 120 min')); + }); + + it('/mode still reads only its first word', async () => { + await post('/mode attention please'); + expect(Integration.findByIdAndUpdate).toHaveBeenCalledWith( + 'integration-1', + { $set: { 'config.relayAllAgentMessages': false } }, + ); + }); + + // F161 item 7: a duration the parse cannot read is answered with usage. + it.each(['90s', '1.5h'])('/mute %s answers with usage and writes nothing', async (arg) => { + await post(`/mute ${arg}`); + expect(Integration.findByIdAndUpdate).not.toHaveBeenCalled(); + expect(telegramService.sendMessage).toHaveBeenCalledWith('bot-token', '42', expect.stringContaining('Usage: /mute')); + }); + + it('/help names the duration forms', async () => { + await post('/help'); + const text = telegramService.sendMessage.mock.calls[0][2]; + expect(text).toContain('30m'); + expect(text).toContain('2h'); + }); + it('/unmute only clears the owner mute and cannot clear an admin pause', async () => { const res = await post('/unmute'); diff --git a/backend/__tests__/unit/scripts/deployDiscordCommands.test.js b/backend/__tests__/unit/scripts/deployDiscordCommands.test.js index 495d320044..c8edfb3bd7 100644 --- a/backend/__tests__/unit/scripts/deployDiscordCommands.test.js +++ b/backend/__tests__/unit/scripts/deployDiscordCommands.test.js @@ -14,7 +14,10 @@ beforeEach(() => { jest.spyOn(console, 'log').mockImplementation(() => {}); }); -afterEach(() => jest.restoreAllMocks()); +afterEach(() => { + jest.restoreAllMocks(); + process.exitCode = undefined; +}); test('one PUT to the global commands endpoint, carrying all seven bridge commands', async () => { axios.put.mockResolvedValue({ status: 200 }); @@ -40,3 +43,45 @@ test('/mode offers exactly the two modes, and /mute takes a duration', () => { const mute = commands.find((c) => c.name === 'mute'); expect(mute.options[0]).toMatchObject({ name: 'duration', type: 3, required: false }); }); + +// F161 item 8: the bridge commands make sense only in a guild channel bound to a pod. +test('every bridge command is offered in guilds only', () => { + const commands = require('../../../config/discordApplicationCommands.json'); + BRIDGE.forEach((name) => { + expect(commands.find((c) => c.name === name).contexts).toEqual([0]); + }); +}); + +// F161 item 5: the boot-time PUT cannot hang boot, and a failure is an exit code. +describe('deploy at boot', () => { + beforeEach(() => { + jest.spyOn(console, 'error').mockImplementation(() => {}); + jest.spyOn(DiscordCommandDeployment.prototype, 'sleep').mockResolvedValue(); + }); + + test('the PUT carries a 10 s timeout', async () => { + axios.put.mockResolvedValue({ status: 200 }); + await new DiscordCommandDeployment().deploy(); + expect(axios.put.mock.calls[0][2].timeout).toBe(10_000); + }); + + test('main sets exit code 1 when every attempt fails', async () => { + axios.put.mockRejectedValue(Object.assign(new Error('timeout of 10000ms exceeded'), { code: 'ECONNABORTED' })); + await DiscordCommandDeployment.main(['deploy']); + expect(axios.put).toHaveBeenCalledTimes(3); + expect(process.exitCode).toBe(1); + }); + + test('main sets exit code 1 when credentials are missing', async () => { + delete process.env.DISCORD_BOT_TOKEN; + await DiscordCommandDeployment.main(['deploy']); + expect(axios.put).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(1); + }); + + test('main leaves the exit code alone on success', async () => { + axios.put.mockResolvedValue({ status: 200 }); + await DiscordCommandDeployment.main(['deploy']); + expect(process.exitCode).toBeUndefined(); + }); +}); diff --git a/backend/__tests__/unit/services/chatRelayCore.test.js b/backend/__tests__/unit/services/chatRelayCore.test.js index 83f6b05ecf..684d87c700 100644 --- a/backend/__tests__/unit/services/chatRelayCore.test.js +++ b/backend/__tests__/unit/services/chatRelayCore.test.js @@ -76,7 +76,8 @@ describe('applyRelayCommand', () => { }); test.each([ - [undefined, 60], ['15', 15], ['0', 60], ['-5', 1], ['99999', 1440], ['soon', 60], + [undefined, 60], ['', 60], ['15', 15], ['0', 60], ['99999', 1440], + ['30m', 30], ['2h', 120], ['2 hours', 120], [' 45 min ', 45], ])('mute %p is %i minutes from now', (arg, minutes) => { const outcome = applyRelayCommand({ name: 'mute', arg }, integration, NOW); const until = new Date(NOW.getTime() + minutes * MIN); @@ -85,6 +86,17 @@ describe('applyRelayCommand', () => { }); }); + // F161 item 7: a unit the parse cannot read is usage, not a guess. Before, + // `90s` fell through to parseInt and muted for 90 minutes. + test.each(['90s', '1.5h', 'soon', '-5', '2 days'])('mute %p is usage and writes nothing', (arg) => { + expect(applyRelayCommand({ name: 'mute', arg }, integration, NOW)).toEqual({ kind: 'mute-usage' }); + }); + + test('a refused caller still gets usage for a bad duration, since nothing would be written', () => { + expect(applyRelayCommand({ name: 'mute', arg: '90s' }, integration, NOW, { mayChangeSettings: false })) + .toEqual({ kind: 'mute-usage' }); + }); + test('unmute unsets only the owner mute', () => { expect(applyRelayCommand({ name: 'unmute' }, integration)) .toEqual({ kind: 'unmute', update: { $unset: { 'config.relayMutedUntil': '' } } }); diff --git a/backend/__tests__/unit/services/discordCommandService.test.js b/backend/__tests__/unit/services/discordCommandService.test.js index e558971b5d..aedfc81b69 100644 --- a/backend/__tests__/unit/services/discordCommandService.test.js +++ b/backend/__tests__/unit/services/discordCommandService.test.js @@ -162,3 +162,48 @@ describe('DiscordCommandService /commonly-summary on a shared channel', () => { ); }); + +// F161 item 1: /discord-enable and /discord-disable change what a shared +// channel feeds into the pod, so they take the rule the bridge commands use: +// only the installer, matched by the Discord id the bot-add proved. +describe('DiscordCommandService /discord-enable and /discord-disable authority', () => { + const INSTALLER = '345678901234567890'; + const STRANGER = '456789012345678901'; + const toggle = async (which, actorId, config = { discordUserId: INSTALLER }) => { + Integration.findOne.mockResolvedValue({ _id: 'integration123', config: { serverId: 'g', channelId: 'c', ...config } }); + Integration.findByIdAndUpdate.mockResolvedValue({}); + const service = new DiscordCommandService({ integrationId: 'integration123' }); + await service.initialize(); + return which === 'enable' ? service.handleEnableCommand(actorId) : service.handleDisableCommand(actorId); + }; + + beforeEach(() => jest.clearAllMocks()); + + it.each([['enable', true], ['disable', false]])('the installer may %s sync', async (which, flag) => { + const result = await toggle(which, INSTALLER); + expect(result.success).toBe(true); + expect(Integration.findByIdAndUpdate).toHaveBeenCalledWith('integration123', { 'config.webhookListenerEnabled': flag }); + }); + + it.each(['enable', 'disable'])('someone else in the channel is refused /discord-%s', async (which) => { + const result = await toggle(which, STRANGER); + expect(Integration.findByIdAndUpdate).not.toHaveBeenCalled(); + expect(result.success).toBe(false); + expect(result.content).toMatch(/only the person who connected this channel/i); + // No ping: the legacy reply carries no allowed_mentions guard. + expect(result.content).not.toContain('<@'); + }); + + it('a caller with no Discord id is refused', async () => { + const result = await toggle('enable', undefined); + expect(Integration.findByIdAndUpdate).not.toHaveBeenCalled(); + expect(result.success).toBe(false); + }); + + it('a row with no proven installer admits no one, and says how to fix it', async () => { + const result = await toggle('disable', INSTALLER, {}); + expect(Integration.findByIdAndUpdate).not.toHaveBeenCalled(); + expect(result.success).toBe(false); + expect(result.content).toMatch(/add the bot again/i); + }); +}); diff --git a/backend/__tests__/unit/services/discordMultiCommandService.test.js b/backend/__tests__/unit/services/discordMultiCommandService.test.js index 98a0f73e63..4ca7e968de 100644 --- a/backend/__tests__/unit/services/discordMultiCommandService.test.js +++ b/backend/__tests__/unit/services/discordMultiCommandService.test.js @@ -107,3 +107,16 @@ describe('discordMultiCommandService', () => { expect(results[1].result.content).toBe('Status B'); }); }); + +// F161 item 1: the multi-pod path hands the caller's Discord id to the toggles. +describe('discordMultiCommandService toggles carry the caller', () => { + it.each([['discord-enable', 'handleEnableCommand'], ['discord-disable', 'handleDisableCommand']])( + '%s passes actorId', + async (commandName, method) => { + const handler = jest.fn().mockResolvedValue({ success: true, content: 'ok' }); + DiscordCommandService.mockImplementation(() => ({ initialize: jest.fn().mockResolvedValue(true), [method]: handler })); + await runCommandForIntegration({ commandName, integration: { _id: 'i1' }, actorId: '345678901234567890' }); + expect(handler).toHaveBeenCalledWith('345678901234567890'); + }, + ); +}); diff --git a/backend/__tests__/unit/services/discordService.test.js b/backend/__tests__/unit/services/discordService.test.js index 4eaffdb7f1..cf47910daf 100644 --- a/backend/__tests__/unit/services/discordService.test.js +++ b/backend/__tests__/unit/services/discordService.test.js @@ -130,6 +130,19 @@ describe('DiscordService', () => { }); }); + // F161 item 1: the single-pod path hands the caller's Discord id to the toggles. + describe('handleSlashCommand passes the caller to the sync toggles', () => { + it.each([ + ['discord-enable', 'handleEnableCommand', { member: { user: { id: '345678901234567890' } } }], + ['discord-disable', 'handleDisableCommand', { user: { id: '345678901234567890' } }], + ])('%s', async (name, method, who) => { + const handler = jest.fn().mockResolvedValue({ success: true, content: 'ok' }); + discordService.commandService = { [method]: handler }; + await discordService.handleSlashCommand(name, { type: 2, data: { name }, ...who }); + expect(handler).toHaveBeenCalledWith('345678901234567890'); + }); + }); + describe('initialization', () => { it('should initialize successfully with valid integration', async () => { const result = await discordService.initialize(); @@ -450,6 +463,18 @@ describe('DiscordService', () => { }); describe('registerSlashCommands', () => { + // F161 item 2: every command is global (config/discordApplicationCommands.json), + // so a guild registration carries none of them. The PUT is a bulk overwrite, + // so an empty one also clears the duplicates older guild registrations left. + it('registers nothing per guild that is already global', async () => { + axios.put.mockResolvedValue({ status: 200 }); + await discordService.registerSlashCommands('guild123'); + const [url, commands] = axios.put.mock.calls[0]; + expect(url).toMatch(/\/guilds\/guild123\/commands$/); + const globalNames = require('../../../config/discordApplicationCommands.json').map((c) => c.name); + expect(commands.filter((c) => globalNames.includes(c.name))).toEqual([]); + }); + it('should register commands successfully', async () => { axios.put.mockResolvedValue({ status: 200 }); diff --git a/backend/config/discordApplicationCommands.json b/backend/config/discordApplicationCommands.json index 3289b48dfe..503850623b 100644 --- a/backend/config/discordApplicationCommands.json +++ b/backend/config/discordApplicationCommands.json @@ -3,6 +3,7 @@ "name": "mode", "description": "Show this channel's relay mode, or switch it", "type": 1, + "contexts": [0], "options": [ { "name": "mode", @@ -19,12 +20,14 @@ { "name": "status", "description": "The pod, relay mode and mute state for this channel", - "type": 1 + "type": 1, + "contexts": [0] }, { "name": "mute", "description": "Pause relay to this channel (default 60 minutes, at most 24 hours)", "type": 1, + "contexts": [0], "options": [ { "name": "duration", @@ -37,22 +40,26 @@ { "name": "unmute", "description": "Resume relay to this channel", - "type": 1 + "type": 1, + "contexts": [0] }, { "name": "tldr", "description": "Post the pod's latest summary here", - "type": 1 + "type": 1, + "contexts": [0] }, { "name": "summary", "description": "Summarize this channel's recent chat into the pod", - "type": 1 + "type": 1, + "contexts": [0] }, { "name": "help", "description": "List the Commonly commands for this channel", - "type": 1 + "type": 1, + "contexts": [0] }, { "name": "commonly-summary", diff --git a/backend/routes/discord.ts b/backend/routes/discord.ts index c190f4b5c4..1b573a2b7a 100644 --- a/backend/routes/discord.ts +++ b/backend/routes/discord.ts @@ -26,7 +26,9 @@ import { listIntegrationsRateLimit, writeIntegrationsRateLimit } from '../middle import { platformIpRateLimit } from '../middleware/platformRateLimit'; import { invalidDiscordIdError, isDiscordSnowflake, isSupplied } from '../utils/discordBinding'; import { assertCanBindSurface, SurfaceBindRefusedError } from '../services/chatRelay/bindSurface'; -import { discordApplicationCommands, handleDiscordRelayCommand, isDiscordRelayCommand } from '../services/discordRelayCommandService'; +import { + discordApplicationCommands, handleDiscordRelayCommand, interactionActorId, isDiscordRelayCommand, +} from '../services/discordRelayCommandService'; import { mintDiscordInstallProof, mintDiscordInstallState, readDiscordInstallProof, readDiscordInstallState, } from '../utils/discordInstallProof'; @@ -221,7 +223,9 @@ router.post('/interactions', async (req: AuthReq & { body: Buffer }, res: Res) = return res.json({ type: 4, data: { content: '❌ Command not recognized.', flags: 64 } }); } - const commandResults = await runDiscordCommandForIntegrations({ commandName: data.name, integrations, guildId, channelId }) as Array<{ integration: typeof integrations[0]; result: { content: string; success: boolean; refused?: boolean } }>; + const commandResults = await runDiscordCommandForIntegrations({ + commandName: data.name, integrations, guildId, channelId, actorId: interactionActorId(interaction), + }) as Array<{ integration: typeof integrations[0]; result: { content: string; success: boolean; refused?: boolean } }>; const header = `🔗 ${integrations.length} pods linked to this Discord channel`; const blocks = commandResults.map(({ integration, result }) => { const podName = (integration.podId as { name?: string })?.name || integration.config?.channelName || `Pod ${integration.podId || integration._id}`; @@ -328,7 +332,9 @@ router.post('/register-commands/:integrationId', auth, async (req: AuthReq, res: const discordService = new DiscordService(integrationId); await discordService.initialize(); const success = await discordService.registerSlashCommands(guildId); - if (success) res.json({ message: 'Slash commands registered successfully', commands: ['/commonly-summary', '/discord-status', '/discord-enable', '/discord-disable'] }); + // Every command is global (F161), so a guild registration clears the + // guild's duplicate copies and adds none. + if (success) res.json({ message: 'Guild command copies cleared; every command is registered globally', commands: [] }); else res.status(500).json({ error: 'Failed to register slash commands' }); } catch (error) { console.error('Error registering slash commands:', error); diff --git a/backend/routes/webhooks/slack.ts b/backend/routes/webhooks/slack.ts index 6d7996bbcc..44108c3807 100644 --- a/backend/routes/webhooks/slack.ts +++ b/backend/routes/webhooks/slack.ts @@ -156,10 +156,11 @@ const commandHelp = 'Use /commonly status, mode mirror|attention, mute [minutes] // with a subcommand. The subcommand is case-insensitive, the argument is taken // as typed, and anything unrecognised — an empty text included — is help. const parseSlackRelayCommand = (text: unknown): RelayCommand => { - const [command, argument] = String(text || '').trim().split(/\s+/, 2); + const [command, argument, ...rest] = String(text || '').trim().split(/\s+/); const normalized = command?.toLowerCase() || 'help'; if (normalized === 'mode') return { name: 'mode', arg: argument }; - if (normalized === 'mute') return { name: 'mute', arg: argument }; + // /mute reads its whole argument (`mute 2 hours`); the others read one word (F161). + if (normalized === 'mute') return { name: 'mute', arg: [argument, ...rest].filter(Boolean).join(' ') || undefined }; if (normalized === 'unmute') return { name: 'unmute' }; if (normalized === 'status') return { name: 'status' }; return { name: 'help' }; @@ -177,6 +178,8 @@ const slackCommandReply = (outcome: RelayCommandOutcome): string => { return `Relay mode: ${outcome.mode}.`; case 'mute': return `Muted for ${outcome.minutes} minutes.`; + case 'mute-usage': + return 'Usage: /commonly mute 30m|2h'; case 'unmute': return 'Relay unmuted.'; case 'status': diff --git a/backend/routes/webhooks/telegram.ts b/backend/routes/webhooks/telegram.ts index 18f9f9f7e7..575e8074a5 100644 --- a/backend/routes/webhooks/telegram.ts +++ b/backend/routes/webhooks/telegram.ts @@ -401,7 +401,7 @@ const HELP_TEXT = [ ' • mirror: every agent message in the pod is copied here', ' • attention: only escalations and the lead agent reach you', '/status — pod, mode, and mute state for this chat', - '/mute [minutes] — pause relay to this chat (default 60); /unmute to resume', + '/mute [30m | 2h] — pause relay to this chat (default 60 minutes; a bare number is minutes); /unmute to resume', '/tldr — latest pod summary', '/summary — summarize recent chat here into the pod', ].join('\n'); @@ -425,6 +425,8 @@ const telegramCommandReply = (outcome: RelayCommandOutcome, podName?: string): s : 'Mode: attention — only escalations and the lead agent reach you.'; case 'mute': return `Muted for ${outcome.minutes} min — nothing relays here until then. /unmute to resume.`; + case 'mute-usage': + return 'Usage: /mute 30m or /mute 2h (a bare number is minutes, at most 24 hours)'; case 'unmute': return 'Unmuted — relay resumes.'; case 'status': { @@ -524,7 +526,8 @@ router.post('/', telegramWebhookRateLimit, async (req: any, res: any) => { return res.sendStatus(200); } - const relayCommand = parseTelegramRelayCommand(command, args[0]); + // /mute reads its whole argument (`/mute 2 hours`); /mode reads one word. + const relayCommand = parseTelegramRelayCommand(command, command === MUTE_COMMAND ? args.join(' ') : args[0]); if (relayCommand) { await handleRelayCommand(chat, integration, relayCommand); return res.sendStatus(200); diff --git a/backend/scripts/deploy-discord-commands.js b/backend/scripts/deploy-discord-commands.js index 02b7ce6331..9ad9a4fa9c 100644 --- a/backend/scripts/deploy-discord-commands.js +++ b/backend/scripts/deploy-discord-commands.js @@ -117,11 +117,14 @@ class DiscordCommandDeployment { try { const url = `https://discord.com/api/v10/applications/${clientId}/commands`; + // Bounded: this runs at boot (start.sh), and an unanswered PUT would + // hold the server's start for as long as the socket stays open. const response = await axios.put(url, commands, { headers: { Authorization: `Bot ${botToken}`, "Content-Type": "application/json", }, + timeout: 10_000, }); if (response.status === 200 || response.status === 201) { @@ -233,18 +236,23 @@ class DiscordCommandDeployment { /** * CLI interface */ -async function main() { - const args = process.argv.slice(2); +async function main(args = process.argv.slice(2)) { const command = args[0]; const deployment = new DiscordCommandDeployment(); + // A failure is an exit code, so start.sh can say so. start.sh does not + // stop the boot on it. + const failIf = (result) => { + if (!result || !result.success) process.exitCode = 1; + }; + switch (command) { case "deploy": - await deployment.deploy(); + failIf(await deployment.deploy()); break; case "verify": - await deployment.verifyDeployment(); + failIf(await deployment.verifyDeployment()); break; case "help": console.log("Discord Command Deployment Script"); @@ -267,13 +275,17 @@ async function main() { break; default: // Default: deploy - await deployment.deploy(); + failIf(await deployment.deploy()); } } // Run if called directly if (require.main === module) { - main().catch(console.error); + main().catch((error) => { + console.error(error); + process.exitCode = 1; + }); } module.exports = DiscordCommandDeployment; +module.exports.main = main; diff --git a/backend/services/chatRelay/chatRelayCore.ts b/backend/services/chatRelay/chatRelayCore.ts index 2247c77111..7bac4e7f4f 100644 --- a/backend/services/chatRelay/chatRelayCore.ts +++ b/backend/services/chatRelay/chatRelayCore.ts @@ -114,6 +114,8 @@ export type RelayCommandOutcome = lead: string | null; } | { kind: 'mute'; minutes: number; until: Date; update: RelayConfigUpdate } + /** A `/mute` duration the parse cannot read (`90s`, `1.5h`): nothing is written. */ + | { kind: 'mute-usage' } | { kind: 'unmute'; update: RelayConfigUpdate } /** A settings change from someone the platform could not prove may make it. */ | { kind: 'refused'; command: RelayCommand['name'] }; @@ -143,17 +145,18 @@ const MUTE_DURATION = /^(\d{1,5}) ?(m|min|mins|minutes?|h|hr|hrs|hours?)?$/i; const MUTE_ARG_MAX = 16; /** - * The minutes a `/mute` argument asks for: a bare number is minutes, and `30m` - * or `2h` carry their unit. Anything else falls back to parseInt, as before - * units were read, so `2h` no longer means two minutes but `soon` still means - * the default. + * The minutes a `/mute` argument asks for: none means the default, a bare + * number is minutes, and `30m` or `2h` carry their unit. Anything else is null, + * which the caller answers with usage: a guess at `90s` or `1.5h` would mute + * for a time nobody asked for (F161). */ -const muteMinutesOf = (arg?: string): number => { - const trimmed = String(arg || '').trim().slice(0, MUTE_ARG_MAX); - const match = MUTE_DURATION.exec(trimmed); - const asked = match - ? parseInt(match[1], 10) * (match[2] && /^h/i.test(match[2]) ? 60 : 1) - : parseInt(trimmed || String(DEFAULT_MUTE_MINUTES), 10); +const muteMinutesOf = (arg?: string): number | null => { + const raw = String(arg || '').trim(); + if (!raw) return DEFAULT_MUTE_MINUTES; + if (raw.length > MUTE_ARG_MAX) return null; + const match = MUTE_DURATION.exec(raw); + if (!match) return null; + const asked = parseInt(match[1], 10) * (match[2] && /^h/i.test(match[2]) ? 60 : 1); return Math.min(Math.max(asked || DEFAULT_MUTE_MINUTES, 1), MAX_MUTE_MINUTES); }; @@ -185,6 +188,7 @@ const decideRelayCommand = ( } if (command.name === 'mute') { const minutes = muteMinutesOf(command.arg); + if (minutes === null) return { kind: 'mute-usage' }; const until = new Date(now.getTime() + minutes * 60_000); return { kind: 'mute', minutes, until, update: { $set: { 'config.relayMutedUntil': until } } }; } diff --git a/backend/services/discordCommandService.ts b/backend/services/discordCommandService.ts index ef70a7a849..ec1a181b29 100644 --- a/backend/services/discordCommandService.ts +++ b/backend/services/discordCommandService.ts @@ -59,6 +59,7 @@ interface IntegrationDoc { serverId?: string; channelId?: string; webhookListenerEnabled?: boolean; + discordUserId?: string; }; populate(field: string, select: string): Promise; } @@ -74,6 +75,27 @@ interface DiscordSummaryData { summaryType: string; } +/** + * F161: channel sync changes what a SHARED channel feeds into the pod, so the + * toggles take the bridge commands' rule (discordRelayCommandService + * `mayChangeRelaySettings`): only the installer, matched by the Discord id the + * bot-add proved. A row bound before that proof existed records no id and + * admits no one; adding the bot again stamps it. Null means allowed. + */ +const syncToggleRefusal = (integration: IntegrationDoc, actorId?: string): CommandResult | null => { + // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require + const { mayChangeRelaySettings } = require('./discordRelayCommandService'); + if (mayChangeRelaySettings(integration, String(actorId || ''))) return null; + // No `<@id>` here: the legacy reply carries no allowed_mentions guard. + return { + success: false, + content: integration.config?.discordUserId + ? 'Only the person who connected this channel to Commonly can turn channel sync on or off.' + : 'Only the person who connected this channel to Commonly can turn channel sync on or off, and this ' + + "connection doesn't record their Discord account. They can add the bot again from Commonly to fix that.", + }; +}; + class DiscordCommandService { private guildId: string | null; @@ -277,7 +299,7 @@ class DiscordCommandService { } } - async handleEnableCommand(): Promise { + async handleEnableCommand(actorId?: string): Promise { try { if (!this.integration) { await this.initialize(); @@ -287,6 +309,9 @@ class DiscordCommandService { return { success: false, content: '❌ Discord integration not found.' }; } + const refusal = syncToggleRefusal(this.integration, actorId); + if (refusal) return refusal; + await Integration.findByIdAndUpdate(this.integration._id, { 'config.webhookListenerEnabled': true, }); @@ -301,7 +326,7 @@ class DiscordCommandService { } } - async handleDisableCommand(): Promise { + async handleDisableCommand(actorId?: string): Promise { try { if (!this.integration) { await this.initialize(); @@ -311,6 +336,9 @@ class DiscordCommandService { return { success: false, content: '❌ Discord integration not found.' }; } + const refusal = syncToggleRefusal(this.integration, actorId); + if (refusal) return refusal; + await Integration.findByIdAndUpdate(this.integration._id, { 'config.webhookListenerEnabled': false, }); diff --git a/backend/services/discordMultiCommandService.ts b/backend/services/discordMultiCommandService.ts index a4412d5729..bf326cfcac 100644 --- a/backend/services/discordMultiCommandService.ts +++ b/backend/services/discordMultiCommandService.ts @@ -26,6 +26,8 @@ interface RunCommandOptions { integration: Integration; guildId?: string; channelId?: string; + /** The caller's Discord id; the sync toggles admit only the installer (F161). */ + actorId?: string; } interface RunMultiCommandOptions { @@ -33,6 +35,7 @@ interface RunMultiCommandOptions { integrations: Integration[]; guildId?: string; channelId?: string; + actorId?: string; } async function runCommandForIntegration({ @@ -40,6 +43,7 @@ async function runCommandForIntegration({ integration, guildId, channelId, + actorId, }: RunCommandOptions): Promise { if (commandName === 'discord-push') { const discordService = new DiscordService(integration._id); @@ -94,12 +98,12 @@ async function runCommandForIntegration({ case 'discord-enable': return { integration, - result: await commandService.handleEnableCommand(), + result: await commandService.handleEnableCommand(actorId), }; case 'discord-disable': return { integration, - result: await commandService.handleDisableCommand(), + result: await commandService.handleDisableCommand(actorId), }; default: return { @@ -117,6 +121,7 @@ async function runDiscordCommandForIntegrations({ integrations, guildId, channelId, + actorId, }: RunMultiCommandOptions): Promise { const results = await Promise.all( integrations.map((integration) => runCommandForIntegration({ @@ -124,6 +129,7 @@ async function runDiscordCommandForIntegrations({ integration, guildId, channelId, + actorId, })), ); diff --git a/backend/services/discordRelayCommandService.ts b/backend/services/discordRelayCommandService.ts index 33b44fae9a..d6a4d3d563 100644 --- a/backend/services/discordRelayCommandService.ts +++ b/backend/services/discordRelayCommandService.ts @@ -127,6 +127,13 @@ const optionValue = (interaction: DiscordInteraction, name: string): string | un }; const NOT_CONNECTED = "This channel isn't connected to a Commonly pod. Connect it from the pod's Connectors page."; +// A row bound here whose connection is not `connected` (F161): the relay's own +// rule (F132), so a command cannot reach a pod the bridge itself would not. +const NOT_ACTIVE = "This channel's connection to Commonly isn't active. Reconnect it from the pod's Connectors page."; +const SETTINGS_NOT_CONNECTED = "This channel isn't connected right now, so its relay settings can't change. " + + "Reconnect it from the pod's Connectors page."; + +const isConnected = (row: DiscordCommandRow): boolean => row.status === 'connected'; const HELP_TEXT = [ '**Commonly commands for this channel**', @@ -176,6 +183,8 @@ const discordCommandReply = (outcome: RelayCommandOutcome, row: DiscordCommandRo case 'mute': return `Muted for ${outcome.minutes} min. Nothing relays here until ${discordTime(outcome.until)}. ` + '/unmute to resume.'; + case 'mute-usage': + return 'Usage: /mute 30m or /mute 2h (a bare number is minutes, at most 24 hours)'; case 'unmute': return 'Unmuted. Relay resumes.'; case 'status': { @@ -237,8 +246,11 @@ const runSettingsCommand = async ( const outcome = applyRelayCommand(command, row, new Date(), { mayChangeSettings: mayChangeRelaySettings(row, actorId), }); - if ('update' in outcome) await Integration.findByIdAndUpdate(row._id, outcome.update); const podName = await podNameOf(row); + // Reads stay open on any bound row (/status names a dropped connection); + // a write waits for the row to be connected again. + if ('update' in outcome && !isConnected(row)) return { podName, text: SETTINGS_NOT_CONNECTED }; + if ('update' in outcome) await Integration.findByIdAndUpdate(row._id, outcome.update); return { podName, text: discordCommandReply(outcome, row, podName) }; })); return reply(joinPerPod(blocks)); @@ -294,34 +306,67 @@ const editDeferredReply = async (interaction: DiscordInteraction, content: strin ); }; +// /summary runs the summarizer, an LLM call, so one row runs it at most once a +// minute (F161). Claimed before the run, so two calls racing in the same +// second still make one. Per process: a second replica keeps its own clock, +// which at worst doubles the rate, never removes the limit. +export const SUMMARY_COOLDOWN_MS = 60_000; +const summaryRanAt = new Map(); + +const claimSummaryRun = (row: DiscordCommandRow): boolean => { + const key = String(row._id); + const now = Date.now(); + const last = summaryRanAt.get(key); + if (last !== undefined && now - last < SUMMARY_COOLDOWN_MS) return false; + summaryRanAt.set(key, now); + // Drop expired entries so the map stays the size of the last minute's rows. + summaryRanAt.forEach((at, id) => { if (now - at >= SUMMARY_COOLDOWN_MS) summaryRanAt.delete(id); }); + return true; +}; + +/** Test seam: forget every row's last /summary. */ +export const resetSummaryCooldowns = (): void => { summaryRanAt.clear(); }; + +const SUMMARY_COOLING = 'A summary ran here less than a minute ago. Try again shortly.'; + // /summary: the channel's buffered chat → a summary → Commonly Bot in the pod, // the path `/discord-push` has always taken. It calls the summarizer, so it // cannot promise Discord's 3 seconds and answers by deferring. -const deferSummary = (interaction: DiscordInteraction, rows: DiscordCommandRow[]): DiscordCommandHandled => ({ - response: { type: 5, data: { flags: EPHEMERAL } }, - after: async () => { - // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require - const { runCommandForIntegration } = require('./discordMultiCommandService'); - const blocks = await Promise.all(rows.map(async (row) => { - const podName = await podNameOf(row); - if (!row.config?.webhookListenerEnabled) { - return { - podName, - text: 'Channel sync is off for this channel, so there is nothing buffered to summarize. ' - + 'Turn it on with /discord-enable.', - }; - } - try { - const { result } = await runCommandForIntegration({ commandName: 'discord-push', integration: row }); - return { podName, text: String(result?.content || '❌ The summary did not run.') }; - } catch (error) { - console.warn(`${LOG} /summary failed integration=${String(row._id)}: ${(error as Error).message}`); - return { podName, text: '❌ The summary did not run. Try again shortly.' }; - } - })); - await editDeferredReply(interaction, joinPerPod(blocks)); - }, -}); +const deferSummary = (interaction: DiscordInteraction, rows: DiscordCommandRow[]): DiscordCommandHandled => { + // Only rows with sync on run the summarizer, so only they take a slot. + const claimed = new Set(rows + .filter((row) => row.config?.webhookListenerEnabled && claimSummaryRun(row)) + .map((row) => String(row._id))); + if (!claimed.size && rows.some((row) => row.config?.webhookListenerEnabled)) { + return { response: reply(SUMMARY_COOLING) }; + } + return { + response: { type: 5, data: { flags: EPHEMERAL } }, + after: async () => { + // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require + const { runCommandForIntegration } = require('./discordMultiCommandService'); + const blocks = await Promise.all(rows.map(async (row) => { + const podName = await podNameOf(row); + if (!row.config?.webhookListenerEnabled) { + return { + podName, + text: 'Channel sync is off for this channel, so there is nothing buffered to summarize. ' + + 'Turn it on with /discord-enable.', + }; + } + if (!claimed.has(String(row._id))) return { podName, text: SUMMARY_COOLING }; + try { + const { result } = await runCommandForIntegration({ commandName: 'discord-push', integration: row }); + return { podName, text: String(result?.content || '❌ The summary did not run.') }; + } catch (error) { + console.warn(`${LOG} /summary failed integration=${String(row._id)}: ${(error as Error).message}`); + return { podName, text: '❌ The summary did not run. Try again shortly.' }; + } + })); + await editDeferredReply(interaction, joinPerPod(blocks)); + }, + }; +}; /** * One bridge command → its interaction response. Only for names @@ -334,8 +379,12 @@ export const handleDiscordRelayCommand = async (interaction: DiscordInteraction) const channelId = String(interaction.channel_id || ''); const rows = await findBoundRows(guildId, channelId); if (!rows.length) return { response: reply(NOT_CONNECTED) }; - if (name === 'tldr') return { response: await runTldr(rows) }; - if (name === 'summary') return deferSummary(interaction, rows); + if (name === 'tldr' || name === 'summary') { + const connected = rows.filter(isConnected); + if (!connected.length) return { response: reply(NOT_ACTIVE) }; + if (name === 'tldr') return { response: await runTldr(connected) }; + return deferSummary(interaction, connected); + } return { response: await runSettingsCommand(interaction, rows) }; }; @@ -347,6 +396,8 @@ module.exports = { isBoundToChannel, mayChangeRelaySettings, findBoundRows, + resetSummaryCooldowns, + SUMMARY_COOLDOWN_MS, renderDiscordTldr, handleDiscordRelayCommand, }; diff --git a/backend/services/discordService.ts b/backend/services/discordService.ts index b27e201584..32488db253 100644 --- a/backend/services/discordService.ts +++ b/backend/services/discordService.ts @@ -821,7 +821,7 @@ class DiscordService { } } - async handleSlashCommand(commandName: string, _interaction: unknown): Promise { + async handleSlashCommand(commandName: string, interaction: unknown): Promise { try { if (!this.commandService) { return { @@ -833,11 +833,16 @@ class DiscordService { const cs = this.commandService as { handleSummaryCommand: () => Promise; handleStatusCommand: () => Promise; - handleEnableCommand: () => Promise; - handleDisableCommand: () => Promise; + handleEnableCommand: (actorId: string) => Promise; + handleDisableCommand: (actorId: string) => Promise; handlePushCommand: (service: DiscordService) => Promise; }; + // The caller's Discord id: `member.user` in a guild, `user` in a DM. The + // sync toggles check it against the installer's (F161). + const who = interaction as { member?: { user?: { id?: string } } | null; user?: { id?: string } | null } | null; + const actorId = String(who?.member?.user?.id || who?.user?.id || ''); + switch (commandName) { case 'commonly-summary': return await cs.handleSummaryCommand(); @@ -846,10 +851,10 @@ class DiscordService { return await cs.handleStatusCommand(); case 'discord-enable': - return await cs.handleEnableCommand(); + return await cs.handleEnableCommand(actorId); case 'discord-disable': - return await cs.handleDisableCommand(); + return await cs.handleDisableCommand(actorId); case 'discord-push': return await cs.handlePushCommand(this); @@ -882,28 +887,12 @@ class DiscordService { console.log(`🔧 Registering commands for guild: ${targetGuildId}`); - const commands = [ - { - name: 'commonly-summary', - description: 'Get the most recent summary from the linked chat pod', - type: 1, - }, - { - name: 'discord-status', - description: 'Show the status of Discord integration', - type: 1, - }, - { - name: 'discord-enable', - description: 'Enable webhook listener for Discord channel', - type: 1, - }, - { - name: 'discord-disable', - description: 'Disable webhook listener for Discord channel', - type: 1, - }, - ]; + // F161: every command, legacy and bridge, is registered globally at boot + // (scripts/deploy-discord-commands.js, config/discordApplicationCommands.json). + // A guild copy showed each legacy command twice in that guild's menu, so + // the guild set is empty, and this bulk overwrite clears the copies older + // registrations left behind. + const commands: Array<{ name: string }> = []; const url = `https://discord.com/api/v10/applications/${config.clientId}/guilds/${targetGuildId}/commands`; @@ -1071,12 +1060,8 @@ class DiscordService { if (response.status === 200) { const registeredCommands = response.data as Array<{ name: string }>; - const expectedCommands = [ - 'commonly-summary', - 'discord-status', - 'discord-enable', - 'discord-disable', - ]; + // Nothing is registered per guild any more (F161); see registerSlashCommands. + const expectedCommands: string[] = []; const foundCommands = registeredCommands.map((cmd) => cmd.name); const missingCommands = expectedCommands.filter( diff --git a/docs/discord/DISCORD.md b/docs/discord/DISCORD.md index cc9e7fff8b..752a6fe6b3 100644 --- a/docs/discord/DISCORD.md +++ b/docs/discord/DISCORD.md @@ -77,7 +77,11 @@ Show the status of your Discord integration. ``` #### **`/discord-enable`** -Enable Discord webhook listener for activity aggregation. +Enable Discord webhook listener for activity aggregation. Only the person who +connected the channel may run it or `/discord-disable`: their Discord id must +equal the row's `config.discordUserId`, the rule the bridge commands use. A row +connected before that id was recorded admits no one until the bot is added +again from Commonly (F161). ``` /discord-enable @@ -319,7 +323,7 @@ These are the Discord routes currently served by Commonly: | `GET` | `/api/discord/callback` | Complete OAuth and redirect to the frontend. | | `POST` | `/api/webhooks/discord` | Receive Discord webhook deliveries and verify their signatures. | | `POST` | `/api/discord/interactions` | Receive signed slash-command interactions. | -| `POST` | `/api/discord/register-commands/:integrationId` | Register commands for one integration. | +| `POST` | `/api/discord/register-commands/:integrationId` | Clear the guild's per-guild command copies; every command is global (F161). | | `POST` | `/api/discord/register-all` | Register commands for all integrations; admin-only. | | `GET` | `/api/discord/health` | Check global command registration and health. | @@ -340,8 +344,13 @@ npm run discord:verify # verify the global command set ``` The deployer requires `DISCORD_CLIENT_ID` and `DISCORD_BOT_TOKEN`, retries a -failed Discord API request up to three times with a five-second delay, and -prints a registration report. It is not an automatic container-start hook. +failed Discord API request up to three times with a five-second delay and a +10-second timeout per request, prints a registration report, and exits 1 on +failure. `backend/start.sh` runs it at boot when the credentials are set and +starts the server whatever it returns, so a Discord outage delays boot by at +most about 45 seconds and never stops it. The set lives in +`backend/config/discordApplicationCommands.json`; the bridge commands carry +`contexts: [0]` (guild channels only). Nothing is registered per guild. For database-backed, integration-scoped registration and inspection: diff --git a/docs/discord/REGISTER_DISCORD_COMMANDS.md b/docs/discord/REGISTER_DISCORD_COMMANDS.md index d866380bf1..0f02d11615 100644 --- a/docs/discord/REGISTER_DISCORD_COMMANDS.md +++ b/docs/discord/REGISTER_DISCORD_COMMANDS.md @@ -27,7 +27,21 @@ take up to an hour to show in every guild. need the caller's Discord id (`member.user.id`) to equal the row's `config.discordUserId`, the installer the OAuth callback proved. Everyone else gets an ephemeral refusal naming that person. Reads are open to anyone in the - bound channel. + bound channel. The legacy `/discord-enable` and `/discord-disable` take the + same rule (F161). +- **Connection state (F161).** `/tldr` and `/summary` act only on a row whose + `status` is `connected`, the relay's own rule. A settings write on a row that + isn't connected says so and writes nothing; `/status` still reads it. +- **Cost (F161).** `/summary` is an LLM run, so each row runs it at most once a + minute (per backend process); a second call inside that minute gets an + ephemeral "try again shortly". +- **Durations (F161).** `/mute` takes nothing (60 minutes), a bare number + (minutes), `30m` or `2h`. Anything else, such as `90s` or `1.5h`, gets usage + and mutes nothing, on Discord, Telegram and Slack alike. Telegram and Slack + read the whole argument, so `/mute 2 hours` is 120 minutes. +- **Per-guild registration (F161).** `POST /api/discord/register-commands/:id` + and `npm run discord:register` now register an empty per-guild set, which + clears the duplicate legacy copies older guild registrations left. - **Binding.** A command acts only on rows whose `serverId` and `channelId` are the ones it was typed in. Unlike the legacy commands, it has no guild-wide fallback. - **Scope.** The install link asks for `applications.commands` already. A guild diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index d847b076b4..03917ee57d 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -2666,7 +2666,8 @@ "discordMirrorHint": "Every agent message is posted in the Discord channel, where everyone in the server can read it.", "discordAttentionHint": "Only escalations and the lead agent are posted in the Discord channel, where everyone in the server can read them.", "discordInboundNote": "Your own messages in the channel reach the pod as you.", - "discordNeedsProof": "Add the bot again from Commonly to turn on relay for this channel." + "discordNeedsProof": "Add the bot again from Commonly to turn on relay for this channel.", + "discordReinstall": "Add the bot again" }, "artifacts": { "title": "Artifacts", diff --git a/frontend/src/i18n/locales/zh-CN.json b/frontend/src/i18n/locales/zh-CN.json index 966716805b..5505ecd590 100644 --- a/frontend/src/i18n/locales/zh-CN.json +++ b/frontend/src/i18n/locales/zh-CN.json @@ -2667,7 +2667,8 @@ "discordMirrorHint": "每条智能体消息都会发到这个 Discord 频道,服务器里的每个人都能看到。", "discordAttentionHint": "只有需要关注的消息和主导智能体的消息会发到这个 Discord 频道,服务器里的每个人都能看到。", "discordInboundNote": "你在频道里发的消息,会以你的身份进入 Pod。", - "discordNeedsProof": "从 Commonly 重新添加一次机器人,就能为这个频道开启中继。" + "discordNeedsProof": "从 Commonly 重新添加一次机器人,就能为这个频道开启中继。", + "discordReinstall": "重新添加机器人" }, "artifacts": { "title": "产物", diff --git a/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx b/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx index 32b61bf586..9ea6d36d25 100644 --- a/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx +++ b/frontend/src/v2/__tests__/ConnectorsPage.channels.test.tsx @@ -1809,6 +1809,39 @@ describe('Connectors page: Discord live relay (F132)', () => { expect(axios.patch).not.toHaveBeenCalled(); }); + // F161 item 10: a Discord row in error said "reconnect to resume" with nothing + // to press. Reconnecting a Discord channel is adding the bot again, through + // the same install link the Connect flow uses. + it('offers a Discord row in error the install link again, and the button leaves for it', async () => { + const realLocation = window.location; + const assign = jest.fn(); + Object.defineProperty(window, 'location', { configurable: true, value: { ...realLocation, assign } }); + try { + const INSTALL_URL = 'https://discord.com/api/oauth2/authorize?client_id=1&response_type=code'; + mockApi({ + connectors: [discordRow({}, { status: 'error' })], + route: (url: string) => (url === '/api/discord/install-link/p2' + ? Promise.resolve({ data: { installUrl: INSTALL_URL, podId: 'p2', status: 'ready' } }) + : undefined), + }); + renderPage(); + const drawer = await openDrawer('Discord'); + expect(within(drawer).getByText('The connection dropped.')).toBeInTheDocument(); + fireEvent.click(within(drawer).getByRole('button', { name: 'Add the bot again' })); + await waitFor(() => expect(assign).toHaveBeenCalledWith(INSTALL_URL)); + expect(axios.get).toHaveBeenCalledWith('/api/discord/install-link/p2', expect.anything()); + } finally { + Object.defineProperty(window, 'location', { configurable: true, value: realLocation }); + } + }); + + it('has the reconnect label in en and zh-CN', () => { + // eslint-disable-next-line global-require + const zh = require('../../i18n/locales/zh-CN.json'); + expect(en.connectors.discordReinstall).toBe('Add the bot again'); + expect(zh.connectors.discordReinstall).toBe('重新添加机器人'); + }); + // CodeRabbit (body, minor): attention mode also sends every lead-agent line. // The row mark has no renderer on the v2 page, so the copy is asserted where // it lives, in both locales. diff --git a/frontend/src/v2/connectors/channelModel.ts b/frontend/src/v2/connectors/channelModel.ts index e3130c529e..831048625b 100644 --- a/frontend/src/v2/connectors/channelModel.ts +++ b/frontend/src/v2/connectors/channelModel.ts @@ -127,7 +127,7 @@ export interface SlackAuthorizeResponse { export type ConnectorAction = | 'manage' | 'show-code' | 'new-code' | 'authorize' | 'confirm' - | 'connect' | 'cancel' | 'retry' | 'retry-remove' | 'pick-pod'; + | 'connect' | 'cancel' | 'retry' | 'retry-remove' | 'pick-pod' | 'reinstall'; export interface ConnectorRow { action: ConnectorAction | null; diff --git a/frontend/src/v2/connectors/useChannelConnectors.ts b/frontend/src/v2/connectors/useChannelConnectors.ts index 467b01d7df..6e83a88cad 100644 --- a/frontend/src/v2/connectors/useChannelConnectors.ts +++ b/frontend/src/v2/connectors/useChannelConnectors.ts @@ -705,9 +705,24 @@ export const useChannelConnectors = () => { const title = connector.config?.chatTitle || connector.config?.teamName || TYPE_LABELS[connector.type] || connector.type; if (connector.status === 'error') { + // F161: reconnecting a Discord channel is adding the bot again, through + // the install link the Connect flow uses. + const isDiscordRow = connector.type === 'discord'; + let errorAction: ConnectorAction | null = null; + let errorActionLabel = ''; + if (isTelegram) { + errorAction = 'new-code'; + errorActionLabel = t('connectors.newCode', { defaultValue: 'New code' }); + } else if (isSlack) { + errorAction = 'authorize'; + errorActionLabel = t('connectors.slackAuthorize', { defaultValue: 'Authorize in Slack' }); + } else if (isDiscordRow && connectorPodId(connector)) { + errorAction = 'reinstall'; + errorActionLabel = t('connectors.discordReinstall', { defaultValue: 'Add the bot again' }); + } return { - action: isTelegram ? 'new-code' : isSlack ? 'authorize' : null, - actionLabel: isTelegram ? t('connectors.newCode', { defaultValue: 'New code' }) : t('connectors.slackAuthorize', { defaultValue: 'Authorize in Slack' }), + action: errorAction, + actionLabel: errorActionLabel, detail: t('connectors.errorReconnect', { defaultValue: 'reconnect to resume' }), dot: 'empty', line: connector.errorMessageUserFacing && connector.errorMessage @@ -1073,6 +1088,10 @@ export const useChannelConnectors = () => { if (!connector) return undefined; if (action === 'new-code') return regenerateCode(connector); if (action === 'authorize') return authorizeSlack(connector, item.key); + if (action === 'reinstall') { + const podId = connectorPodId(connector); + return podId ? linkInstall(connector.type, String(podId)) : undefined; + } // The row opens the aside where the active pod's lead can be chosen. // Sending a confirm here would silently omit that choice. if (action === 'confirm') return undefined; From 667ca40c119d3d3f4c4c4e71d781c58c84a20b26 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 18:29:43 -0800 Subject: [PATCH 2/3] fix(discord): re-adding the bot retires the dropped row; command deploy bounded; verify checks shape (#3470) CI and CodeRabbit on e111c857: - The ReDoS mute input now expects usage, still parsed in under 50 ms. - Each boot-time PUT attempt has a 15 s wall-clock AbortController covering the response body, cleared in finally. - A Discord bind on the same pod, guild and channel as one of the caller's rows in error or disconnected retires that row as Remove would (grants revoked, webhook deleted, row inactive); best effort, never fails the bind. - The empty per-guild PUT runs only once the global bridge commands exist; otherwise the guild keeps its commands and the reply names what's missing. - Per-guild verify fails while the guild still holds commands; discord:verify compares contexts and options, not only names; exit codes tested. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DPFVAooJnqCSwrdxVw --- .../service/discordInstallBind.test.js | 71 +++++++++++++++ .../scripts/deployDiscordCommands.test.js | 90 +++++++++++++++++++ .../services/chatRelayCore.commands.test.js | 13 ++- .../unit/services/discordService.test.js | 41 +++++++++ backend/routes/discord.ts | 4 +- backend/routes/integrations.ts | 41 +++++++++ backend/scripts/deploy-discord-commands.js | 60 ++++++++++--- backend/services/discordService.ts | 29 +++++- docs/discord/REGISTER_DISCORD_COMMANDS.md | 12 ++- 9 files changed, 345 insertions(+), 16 deletions(-) diff --git a/backend/__tests__/service/discordInstallBind.test.js b/backend/__tests__/service/discordInstallBind.test.js index 66b59f0a99..7309e75ca8 100644 --- a/backend/__tests__/service/discordInstallBind.test.js +++ b/backend/__tests__/service/discordInstallBind.test.js @@ -48,6 +48,7 @@ jest.mock('../../models/Pod', () => ({ findById: jest.fn(), find: jest.fn() })); jest.mock('../../services/discordService', () => jest.fn().mockImplementation(() => ({ initialize: jest.fn().mockResolvedValue(true), connect: jest.fn().mockResolvedValue(true), + disconnect: jest.fn().mockResolvedValue(true), }))); jest.mock('../../services/connectorSecrets', () => ({ put: jest.fn().mockResolvedValue('secret-ref-1'), @@ -328,3 +329,73 @@ describe('binding a Discord channel takes authority over the pod', () => { expect((await bindAs('member-token', proof)).status).toBe(201); }); }); + +// F161 (CodeRabbit 4475816316): "Add the bot again" on a row in error starts a +// fresh install. The new bind retires the caller's dropped row on the same pod, +// guild and channel the way Remove would, so the page doesn't show two rows. +describe('a fresh bind replaces the dropped row it reconnects', () => { + const OTHER_CHANNEL = '456789012345678901'; + const seedRow = async (over = {}) => { + const row = await Integration.create({ + podId: POD_ID, + type: 'discord', + createdBy: OWNER, + status: 'error', + config: { serverId: GUILD, channelId: CHANNEL, channelName: 'general' }, + ...over, + }); + await DiscordIntegration.create({ + integrationId: row._id, serverId: GUILD, serverName: 'Commonly Dev', channelId: row.config.channelId, channelName: 'general', webhookId: 'old-hook', + }); + return row; + }; + + const bindViaInstall = async () => { + const link = await request(app).get(`/api/discord/install-link/${POD_ID}`).set('Authorization', 'Bearer owner-token'); + const authorize = new URL(link.body.installUrl); + const back = new URL(authorize.searchParams.get('redirect_uri')); + const cb = await request(app) + .get(`${back.pathname}?code=the-code&state=${authorize.searchParams.get('state')}&guild_id=${GUILD}`); + const proof = new URL(cb.headers.location).searchParams.get('install'); + return request(app) + .post('/api/integrations') + .set('Authorization', 'Bearer owner-token') + .send({ + podId: POD_ID, + type: 'discord', + config: { serverId: GUILD, serverName: 'Commonly Dev', channelId: CHANNEL, channelName: 'general' }, + install: proof, + }); + }; + + test.each(['error', 'disconnected'])('a %s row on the same channel ends up inactive, its webhook record gone', async (status) => { + const old = await seedRow({ status }); + const bind = await bindViaInstall(); + expect(bind.status).toBe(201); + const retired = await Integration.findById(old._id).lean(); + expect(retired.isActive).toBe(false); + expect(retired.status).toBe('disconnected'); + expect(await DiscordIntegration.findOne({ integrationId: old._id })).toBeNull(); + const DiscordService = require('../../services/discordService'); + const retiredService = DiscordService.mock.results + .map((r, i) => ({ value: r.value, id: String(DiscordService.mock.calls[i][0]) })) + .find((r) => r.id === String(old._id)); + expect(retiredService.value.disconnect).toHaveBeenCalled(); + const live = await Integration.find({ podId: POD_ID, type: 'discord', isActive: true }).lean(); + expect(live.map((r) => String(r._id))).toEqual([String(bind.body.integration._id)]); + }); + + test('rows the bind does not replace stay as they are', async () => { + const otherChannel = await seedRow({ config: { serverId: GUILD, channelId: OTHER_CHANNEL } }); + const connected = await seedRow({ status: 'connected' }); + const someoneElses = await seedRow({ createdBy: MEMBER }); + const bind = await bindViaInstall(); + expect(bind.status).toBe(201); + for (const row of [otherChannel, connected, someoneElses]) { + // eslint-disable-next-line no-await-in-loop + const after = await Integration.findById(row._id).lean(); + expect(after.isActive).toBe(true); + expect(after.status).toBe(row.status); + } + }); +}); diff --git a/backend/__tests__/unit/scripts/deployDiscordCommands.test.js b/backend/__tests__/unit/scripts/deployDiscordCommands.test.js index c8edfb3bd7..24cdc9d448 100644 --- a/backend/__tests__/unit/scripts/deployDiscordCommands.test.js +++ b/backend/__tests__/unit/scripts/deployDiscordCommands.test.js @@ -85,3 +85,93 @@ describe('deploy at boot', () => { expect(process.exitCode).toBeUndefined(); }); }); + +// F161 (CodeRabbit 4475816306): axios's `timeout` stops counting once headers +// arrive, so a body that trickles forever would still hold boot. Each attempt +// has a wall-clock deadline over the whole request and response. +describe('the per-attempt deadline', () => { + beforeEach(() => { + jest.spyOn(console, 'error').mockImplementation(() => {}); + jest.spyOn(DiscordCommandDeployment.prototype, 'sleep').mockResolvedValue(); + jest.useFakeTimers(); + }); + afterEach(() => jest.useRealTimers()); + + // A request that never settles unless its signal aborts it. + const hangUntilAborted = (_url, _body, options) => new Promise((_resolve, reject) => { + options.signal.addEventListener('abort', () => reject(Object.assign(new Error('canceled'), { code: 'ERR_CANCELED' }))); + }); + + test('a hanging response is aborted at 15 s, and three of them fail the deploy', async () => { + axios.put.mockImplementation(hangUntilAborted); + const done = new DiscordCommandDeployment().deploy(); + await jest.advanceTimersByTimeAsync(14_999); + expect(axios.put).toHaveBeenCalledTimes(1); + expect(axios.put.mock.calls[0][2].signal.aborted).toBe(false); + await jest.advanceTimersByTimeAsync(1); + expect(axios.put.mock.calls[0][2].signal.aborted).toBe(true); + await jest.advanceTimersByTimeAsync(30_000); + const result = await done; + expect(axios.put).toHaveBeenCalledTimes(3); + expect(result.success).toBe(false); + }); + + test('a settled attempt leaves no deadline timer behind', async () => { + axios.put.mockResolvedValue({ status: 200 }); + await new DiscordCommandDeployment().deploy(); + expect(jest.getTimerCount()).toBe(0); + }); +}); + +// F161 items 6 and 7: verify's exit status, and a registered command whose +// shape has gone stale (contexts or options) counts as a failure. +describe('verify', () => { + const commands = require('../../../config/discordApplicationCommands.json'); + const live = () => JSON.parse(JSON.stringify(commands)).map((c) => ({ id: `id-${c.name}`, application_id: 'app-1', version: '1', ...c })); + + beforeEach(() => { + jest.spyOn(console, 'error').mockImplementation(() => {}); + }); + + test('a matching set exits 0', async () => { + axios.get.mockResolvedValue({ status: 200, data: live() }); + await DiscordCommandDeployment.main(['verify']); + expect(process.exitCode).toBeUndefined(); + }); + + test('a missing command exits 1', async () => { + axios.get.mockResolvedValue({ status: 200, data: live().filter((c) => c.name !== 'tldr') }); + await DiscordCommandDeployment.main(['verify']); + expect(process.exitCode).toBe(1); + }); + + test('an API error exits 1', async () => { + axios.get.mockRejectedValue(new Error('503')); + await DiscordCommandDeployment.main(['verify']); + expect(process.exitCode).toBe(1); + }); + + test('a command registered without its guild-only context is stale', async () => { + const data = live().map((c) => (c.name === 'mute' ? { ...c, contexts: null } : c)); + axios.get.mockResolvedValue({ status: 200, data }); + const result = await new DiscordCommandDeployment().verifyDeployment(); + expect(result.success).toBe(false); + expect(result.staleCommands).toEqual(['mute']); + }); + + test('a command whose choices changed is stale', async () => { + const data = live().map((c) => (c.name === 'mode' + ? { ...c, options: [{ ...c.options[0], choices: [{ name: 'mirror', value: 'mirror' }] }] } + : c)); + axios.get.mockResolvedValue({ status: 200, data }); + const result = await new DiscordCommandDeployment().verifyDeployment(); + expect(result.staleCommands).toEqual(['mode']); + }); + + test('fields Discord adds on its own are not a difference', async () => { + const data = live().map((c) => ({ ...c, dm_permission: true, nsfw: false, integration_types: [0] })); + axios.get.mockResolvedValue({ status: 200, data }); + const result = await new DiscordCommandDeployment().verifyDeployment(); + expect(result.success).toBe(true); + }); +}); diff --git a/backend/__tests__/unit/services/chatRelayCore.commands.test.js b/backend/__tests__/unit/services/chatRelayCore.commands.test.js index 3439c8a8c0..83e4c9bdd4 100644 --- a/backend/__tests__/unit/services/chatRelayCore.commands.test.js +++ b/backend/__tests__/unit/services/chatRelayCore.commands.test.js @@ -25,12 +25,23 @@ describe('parseRelayCommand', () => { test.each([ ['30m', 30], ['30 min', 30], ['2h', 120], ['1 hour', 60], ['48h', 1440], ['45', 45], ['0m', 60], - [' 2h ', 120], ['0' + '\t'.repeat(50000) + 'x', 60], + [' 2h ', 120], ])('mute %p is %i minutes', (arg, minutes) => { const outcome = applyRelayCommand({ name: 'mute', arg }, integration, NOW); expect(outcome).toMatchObject({ kind: 'mute', minutes, until: new Date(NOW.getTime() + minutes * MIN) }); }); +// ReDoS guard (#3469): a long run of whitespace before an unreadable unit must +// stay linear. Since F161 an unreadable duration is usage, not the default. +test('a 50,000-tab duration is usage, answered quickly', () => { + const arg = '0' + '\t'.repeat(50000) + 'x'; + const started = process.hrtime.bigint(); + const outcome = applyRelayCommand({ name: 'mute', arg }, integration, NOW); + const elapsedMs = Number(process.hrtime.bigint() - started) / 1e6; + expect(outcome).toEqual({ kind: 'mute-usage' }); + expect(elapsedMs).toBeLessThan(50); +}); + describe('the authority gate', () => { test.each([ [{ name: 'mode', arg: 'mirror' }], diff --git a/backend/__tests__/unit/services/discordService.test.js b/backend/__tests__/unit/services/discordService.test.js index cf47910daf..817bda26eb 100644 --- a/backend/__tests__/unit/services/discordService.test.js +++ b/backend/__tests__/unit/services/discordService.test.js @@ -143,6 +143,23 @@ describe('DiscordService', () => { }); }); + // F161 item 4: nothing is expected per guild, so a guild that still holds + // commands is not verified. + describe('verifyCommandRegistration', () => { + it('passes for a guild with no commands', async () => { + axios.get.mockResolvedValue({ status: 200, data: [] }); + const result = await discordService.verifyCommandRegistration('guild123'); + expect(result.success).toBe(true); + }); + + it('fails for a guild that still holds copies', async () => { + axios.get.mockResolvedValue({ status: 200, data: [{ name: 'discord-enable' }] }); + const result = await discordService.verifyCommandRegistration('guild123'); + expect(result.success).toBe(false); + expect(result.unexpectedCommands).toEqual(['discord-enable']); + }); + }); + describe('initialization', () => { it('should initialize successfully with valid integration', async () => { const result = await discordService.initialize(); @@ -463,6 +480,30 @@ describe('DiscordService', () => { }); describe('registerSlashCommands', () => { + const GLOBAL_SET = require('../../../config/discordApplicationCommands.json'); + const GLOBAL_URL = 'https://discord.com/api/v10/applications/test-client-id/commands'; + beforeEach(() => { + axios.get.mockImplementation(async (url) => (url === GLOBAL_URL ? { status: 200, data: GLOBAL_SET } : { status: 404 })); + }); + + // F161 item 5: clearing a guild's copies while the global set is missing + // (a boot deploy that failed) would leave that guild with no commands. + it('keeps the guild set when the global commands are not registered', async () => { + axios.get.mockResolvedValue({ status: 200, data: GLOBAL_SET.filter((c) => c.name !== 'mute') }); + const result = await discordService.registerSlashCommands('guild123'); + expect(result).toBe(false); + expect(axios.put).not.toHaveBeenCalled(); + expect(discordService.lastRegistrationError).toMatch(/global commands are not all registered/i); + expect(discordService.lastRegistrationError).toContain('mute'); + }); + + it('keeps the guild set when the global list cannot be read', async () => { + axios.get.mockRejectedValue(new Error('503')); + const result = await discordService.registerSlashCommands('guild123'); + expect(result).toBe(false); + expect(axios.put).not.toHaveBeenCalled(); + }); + // F161 item 2: every command is global (config/discordApplicationCommands.json), // so a guild registration carries none of them. The PUT is a bulk overwrite, // so an empty one also clears the duplicates older guild registrations left. diff --git a/backend/routes/discord.ts b/backend/routes/discord.ts index 1b573a2b7a..dc233503f3 100644 --- a/backend/routes/discord.ts +++ b/backend/routes/discord.ts @@ -335,7 +335,9 @@ router.post('/register-commands/:integrationId', auth, async (req: AuthReq, res: // Every command is global (F161), so a guild registration clears the // guild's duplicate copies and adds none. if (success) res.json({ message: 'Guild command copies cleared; every command is registered globally', commands: [] }); - else res.status(500).json({ error: 'Failed to register slash commands' }); + // The service's reason, e.g. the global set is not in place so the guild's + // commands were kept (F161). Its text names no credential. + else res.status(500).json({ error: discordService.lastRegistrationError || 'Failed to register slash commands' }); } catch (error) { console.error('Error registering slash commands:', error); res.status(500).json({ error: 'Failed to register slash commands' }); diff --git a/backend/routes/integrations.ts b/backend/routes/integrations.ts index afc69108d7..f1aedcd485 100644 --- a/backend/routes/integrations.ts +++ b/backend/routes/integrations.ts @@ -493,6 +493,44 @@ router.get('/:podId', listIntegrationsRateLimit, auth, async (req: AuthReq, res: } }); +// F161: "Add the bot again" on a Discord row that dropped starts a fresh +// install, and the bind below writes a new row. The caller's own dropped rows +// on the same pod, guild and channel are then retired the way Remove retires a +// row: grants revoked, webhook deleted, platform record gone. The row itself is +// kept, inactive, rather than deleted. Best effort: a failure here leaves a +// stale row, never a failed bind. +const retireDroppedDiscordRows = async ( + keepId: unknown, podId: unknown, userId: string, serverId: string, channelId: string, +): Promise => { + let dropped: Array<{ _id: unknown }> = []; + try { + dropped = await Integration.find({ + _id: { $ne: keepId }, + type: 'discord', + isActive: true, + podId, + createdBy: userId, + 'config.serverId': serverId, + 'config.channelId': channelId, + status: { $in: ['error', 'disconnected'] }, + }) as Array<{ _id: unknown }>; + } catch (error) { + console.warn(`Discord rebind: could not look up dropped rows: ${(error as Error).message}`); + return; + } + await Promise.all(dropped.map(async (row) => { + try { + await revokeConnectionGrants({ connection: row, revokedBy: userId }); + const service = new DiscordService(row._id); + if (await service.initialize()) await service.disconnect(); + await DiscordIntegration.findOneAndDelete({ integrationId: row._id }); + await Integration.findByIdAndUpdate(row._id, { $set: { isActive: false, status: 'disconnected' } }); + } catch (error) { + console.warn(`Discord rebind: could not retire integration ${String(row._id)}: ${(error as Error).message}`); + } + })); +}; + router.post('/', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: Res) => { try { const { podId, type, config } = (req.body || {}) as { podId?: string; type?: unknown; config?: Record }; @@ -654,6 +692,9 @@ router.post('/', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: Res if (!initialized) return res.status(500).json({ message: 'Failed to initialize integration' }); const connected = await service.connect(); if (!connected) console.warn('Integration initialized but failed to connect'); + await retireDroppedDiscordRows( + integration._id, podId, String(req.user?.id), String(config.serverId), String(config.channelId), + ); } res.status(201).json({ integration, platformIntegration }); } catch (error) { diff --git a/backend/scripts/deploy-discord-commands.js b/backend/scripts/deploy-discord-commands.js index 9ad9a4fa9c..d821930bc6 100644 --- a/backend/scripts/deploy-discord-commands.js +++ b/backend/scripts/deploy-discord-commands.js @@ -1,6 +1,23 @@ const axios = require("axios"); const DISCORD_APPLICATION_COMMANDS = require("../config/discordApplicationCommands.json"); +// Each registration attempt, request and response together, ends by this. +const ATTEMPT_DEADLINE_MS = 15_000; + +// The parts of a command that change what Discord shows: where it is offered +// (`contexts`) and its options. Fields Discord adds itself (id, version, +// dm_permission, integration_types, ...) are not compared. +const optionShape = (options) => (options || []).map((o) => ({ + name: o.name, + type: o.type, + required: Boolean(o.required), + choices: (o.choices || []).map((c) => c.value), +})); +const commandShape = (cmd) => JSON.stringify({ + contexts: cmd.contexts || null, + options: optionShape(cmd.options), +}); + /** * Discord Command Deployment Script * Registers global slash commands with Discord API @@ -117,15 +134,25 @@ class DiscordCommandDeployment { try { const url = `https://discord.com/api/v10/applications/${clientId}/commands`; - // Bounded: this runs at boot (start.sh), and an unanswered PUT would - // hold the server's start for as long as the socket stays open. - const response = await axios.put(url, commands, { - headers: { - Authorization: `Bot ${botToken}`, - "Content-Type": "application/json", - }, - timeout: 10_000, - }); + // Bounded: this runs at boot (start.sh). axios's `timeout` covers the + // wait for headers only, so a body that trickles forever would still + // hold the server's start; the abort is a wall-clock deadline over the + // whole request and response. + const controller = new AbortController(); + const deadline = setTimeout(() => controller.abort(), ATTEMPT_DEADLINE_MS); + let response; + try { + response = await axios.put(url, commands, { + headers: { + Authorization: `Bot ${botToken}`, + "Content-Type": "application/json", + }, + timeout: 10_000, + signal: controller.signal, + }); + } finally { + clearTimeout(deadline); + } if (response.status === 200 || response.status === 201) { console.log(`✅ Discord API returned status ${response.status}`); @@ -211,17 +238,26 @@ class DiscordCommandDeployment { const missingCommands = expectedCommands.filter( (cmd) => !foundCommands.includes(cmd), ); - - if (missingCommands.length === 0) { + // Present but registered from an older file: a deploy is still due. + const staleCommands = DISCORD_APPLICATION_COMMANDS + .filter((cmd) => { + const found = registeredCommands.find((r) => r.name === cmd.name); + return found && commandShape(found) !== commandShape(cmd); + }) + .map((cmd) => cmd.name); + + if (missingCommands.length === 0 && staleCommands.length === 0) { console.log( `✅ All ${expectedCommands.length} commands verified globally`, ); return { success: true, registeredCommands: foundCommands }; } - console.log(`❌ Missing commands: ${missingCommands.join(", ")}`); + if (missingCommands.length) console.log(`❌ Missing commands: ${missingCommands.join(", ")}`); + if (staleCommands.length) console.log(`❌ Stale commands (contexts or options differ): ${staleCommands.join(", ")}`); return { success: false, missingCommands, + staleCommands, registeredCommands: foundCommands, }; } diff --git a/backend/services/discordService.ts b/backend/services/discordService.ts index 32488db253..13899f1809 100644 --- a/backend/services/discordService.ts +++ b/backend/services/discordService.ts @@ -20,6 +20,8 @@ const { resolveDiscordWebhookUrl, hasDiscordWebhookUrl } = require('../utils/dis const connectorSecrets = require('./connectorSecrets'); // eslint-disable-next-line global-require const { DISCORD_WEBHOOK_URL } = require('./connectorSecretKinds'); +// eslint-disable-next-line global-require +const DISCORD_APPLICATION_COMMANDS = require('../config/discordApplicationCommands.json') as Array<{ name: string }>; interface FetchMessagesOptions { channelId?: string; @@ -127,6 +129,8 @@ interface VerifyCommandResult { success: boolean; registeredCommands?: string[]; missingCommands?: string[]; + /** Commands the guild holds that nothing expects there (F161). */ + unexpectedCommands?: string[]; totalExpected?: number; totalFound?: number; error?: string; @@ -156,6 +160,8 @@ class DiscordService { clientReady: boolean; commandService: unknown; integration: IntegrationDoc | null; + /** Why the last registerSlashCommands returned false, for the route to show. */ + lastRegistrationError: string | null = null; static async fetchMessages(options: FetchMessagesOptions = {}): Promise { const { @@ -894,6 +900,23 @@ class DiscordService { // registrations left behind. const commands: Array<{ name: string }> = []; + // Clearing the guild's copies is safe only while the global set is in + // place; after a boot deploy that failed, the copies are all the guild + // has. A read that fails counts as not in place. + const globalResponse = await axios.get( + `https://discord.com/api/v10/applications/${config.clientId}/commands`, + { headers: { Authorization: `Bot ${config.botToken}` }, timeout: 10_000 }, + ) as { status?: number; data?: Array<{ name?: string }> } | undefined; + const globalNames = new Set((globalResponse?.status === 200 && Array.isArray(globalResponse.data) + ? globalResponse.data : []).map((cmd) => cmd?.name)); + const missingGlobal = DISCORD_APPLICATION_COMMANDS + .map((cmd) => cmd.name) + .filter((name) => !globalNames.has(name)); + if (missingGlobal.length) { + throw new Error(`The global commands are not all registered (missing: ${missingGlobal.join(', ')}), ` + + "so this guild's commands were kept. Run npm run discord:deploy first."); + } + const url = `https://discord.com/api/v10/applications/${config.clientId}/guilds/${targetGuildId}/commands`; const response = await axios.put(url, commands, { @@ -922,6 +945,7 @@ class DiscordService { } catch (error) { const err = error as { message?: string }; console.error('❌ Failed to register commands:', err.message); + this.lastRegistrationError = err.message || 'Failed to register slash commands'; if (this.integration) { await Integration.findByIdAndUpdate(this.integrationId, { @@ -1067,9 +1091,12 @@ class DiscordService { const missingCommands = expectedCommands.filter( (cmd) => !foundCommands.includes(cmd), ); + // With nothing expected, a guild that still holds copies is the failure. + const unexpectedCommands = foundCommands.filter((cmd) => !expectedCommands.includes(cmd)); return { - success: missingCommands.length === 0, + success: missingCommands.length === 0 && unexpectedCommands.length === 0, + unexpectedCommands, registeredCommands: foundCommands, missingCommands, totalExpected: expectedCommands.length, diff --git a/docs/discord/REGISTER_DISCORD_COMMANDS.md b/docs/discord/REGISTER_DISCORD_COMMANDS.md index 0f02d11615..e8863c32d8 100644 --- a/docs/discord/REGISTER_DISCORD_COMMANDS.md +++ b/docs/discord/REGISTER_DISCORD_COMMANDS.md @@ -41,7 +41,17 @@ take up to an hour to show in every guild. read the whole argument, so `/mute 2 hours` is 120 minutes. - **Per-guild registration (F161).** `POST /api/discord/register-commands/:id` and `npm run discord:register` now register an empty per-guild set, which - clears the duplicate legacy copies older guild registrations left. + clears the duplicate legacy copies older guild registrations left. They first + read the global set and keep the guild's commands if any global command is + missing (a boot deploy that failed). +- **Boot deploy (F161).** Each PUT attempt has a 15 s wall-clock deadline + (`AbortController`) over request and response; axios's own `timeout` stops + counting once headers arrive. `discord:verify` exits 1 when a command is + missing or registered with different `contexts` or options. +- **Reconnect (F161).** "Add the bot again" on a dropped row is a fresh install. + When it binds the same pod, guild and channel, the caller's rows there in + `error` or `disconnected` are retired as Remove would (grants revoked, webhook + deleted, platform record gone) and kept inactive. - **Binding.** A command acts only on rows whose `serverId` and `channelId` are the ones it was typed in. Unlike the legacy commands, it has no guild-wide fallback. - **Scope.** The install link asks for `applications.commands` already. A guild From ced5ed1e3f4ce9658c6722019401430e9dbbcffe Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 18:35:53 -0800 Subject: [PATCH 3/3] fix(discord): the dropped-row lookup takes only shape-checked strings (#3470) CodeQL: the retire query was built from request values. Guild and channel must be snowflakes and the pod an ObjectId string, all coerced to plain strings, before the filter is built; anything else skips the retire. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DPFVAooJnqCSwrdxVw --- backend/routes/integrations.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/backend/routes/integrations.ts b/backend/routes/integrations.ts index f1aedcd485..6dd3290297 100644 --- a/backend/routes/integrations.ts +++ b/backend/routes/integrations.ts @@ -502,16 +502,22 @@ router.get('/:podId', listIntegrationsRateLimit, auth, async (req: AuthReq, res: const retireDroppedDiscordRows = async ( keepId: unknown, podId: unknown, userId: string, serverId: string, channelId: string, ): Promise => { + // Every value comes from the request path: pin each to the shape it must + // have, so the filter can only ever hold plain strings. + const guild = String(serverId); + const channel = String(channelId); + const pod = String(podId); + if (!/^\d{17,20}$/.test(guild) || !/^\d{17,20}$/.test(channel) || !/^[a-f0-9]{24}$/i.test(pod)) return; let dropped: Array<{ _id: unknown }> = []; try { dropped = await Integration.find({ - _id: { $ne: keepId }, + _id: { $ne: String(keepId) }, type: 'discord', isActive: true, - podId, - createdBy: userId, - 'config.serverId': serverId, - 'config.channelId': channelId, + podId: pod, + createdBy: String(userId), + 'config.serverId': guild, + 'config.channelId': channel, status: { $in: ['error', 'disconnected'] }, }) as Array<{ _id: unknown }>; } catch (error) {