From b936877afa0378f028f62ecf092f5b609630ee4a Mon Sep 17 00:00:00 2001 From: AnExiledDev Date: Thu, 12 Feb 2026 03:29:52 +0000 Subject: [PATCH 1/3] =?UTF-8?q?fix:=20address=20code=20review=20findings?= =?UTF-8?q?=20from=20PRs=20#12=E2=80=93#18?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve six issues identified during review of the last five merged PRs: - Wrap Discord message sends in try-catch with user-friendly fallback - Invert NODE_ENV check to hide error details by default - Remove duplicate unhandledRejection handler from index.js - Remove dead graceful-shutdown request tracking code - Null out ChimeIn AbortController in finally block - Add throttled eviction to welcome activity Map --- src/commands/config.js | 6 ++-- src/index.js | 55 ++++------------------------------ src/modules/chimeIn.js | 1 + src/modules/events.js | 56 ++++++++++++++++++++++------------- src/modules/welcome.js | 29 ++++++++++++++++++ tests/commands/config.test.js | 24 +++++++++++++++ tests/index.test.js | 22 -------------- tests/modules/events.test.js | 42 ++++++++++++++++++++++++++ tests/modules/welcome.test.js | 37 +++++++++++++++++++++++ 9 files changed, 176 insertions(+), 96 deletions(-) diff --git a/src/commands/config.js b/src/commands/config.js index 8ca0cf78..e1cfe2c3 100644 --- a/src/commands/config.js +++ b/src/commands/config.js @@ -257,7 +257,7 @@ async function handleView(interaction) { await interaction.reply({ embeds: [embed], ephemeral: true }); } catch (err) { const safeMessage = - process.env.NODE_ENV === 'production' ? 'An internal error occurred.' : err.message; + process.env.NODE_ENV === 'development' ? err.message : 'An internal error occurred.'; await interaction.reply({ content: `❌ Failed to load config: ${safeMessage}`, ephemeral: true, @@ -311,7 +311,7 @@ async function handleSet(interaction) { await interaction.editReply({ embeds: [embed] }); } catch (err) { const safeMessage = - process.env.NODE_ENV === 'production' ? 'An internal error occurred.' : err.message; + process.env.NODE_ENV === 'development' ? err.message : 'An internal error occurred.'; const content = `❌ Failed to set config: ${safeMessage}`; if (interaction.deferred) { await interaction.editReply({ content }); @@ -346,7 +346,7 @@ async function handleReset(interaction) { await interaction.editReply({ embeds: [embed] }); } catch (err) { const safeMessage = - process.env.NODE_ENV === 'production' ? 'An internal error occurred.' : err.message; + process.env.NODE_ENV === 'development' ? err.message : 'An internal error occurred.'; const content = `❌ Failed to reset config: ${safeMessage}`; if (interaction.deferred) { await interaction.editReply({ content }); diff --git a/src/index.js b/src/index.js index a6d94b0b..3137ad64 100644 --- a/src/index.js +++ b/src/index.js @@ -67,27 +67,6 @@ client.commands = new Collection(); // Initialize health monitor const healthMonitor = HealthMonitor.getInstance(); -// Track pending AI requests for graceful shutdown -const pendingRequests = new Set(); - -/** - * Register a pending request for tracking - * @returns {Symbol} Request ID to use for cleanup - */ -export function registerPendingRequest() { - const requestId = Symbol('request'); - pendingRequests.add(requestId); - return requestId; -} - -/** - * Remove a pending request from tracking - * @param {Symbol} requestId - Request ID to remove - */ -export function removePendingRequest(requestId) { - pendingRequests.delete(requestId); -} - /** * Save conversation history to disk */ @@ -224,31 +203,14 @@ client.on('interactionCreate', async (interaction) => { async function gracefulShutdown(signal) { info('Shutdown initiated', { signal }); - // 1. Wait for pending requests with timeout - const SHUTDOWN_TIMEOUT = 10000; // 10 seconds - if (pendingRequests.size > 0) { - info('Waiting for pending requests', { count: pendingRequests.size }); - const startTime = Date.now(); - - while (pendingRequests.size > 0 && Date.now() - startTime < SHUTDOWN_TIMEOUT) { - await new Promise((resolve) => setTimeout(resolve, 100)); - } - - if (pendingRequests.size > 0) { - warn('Shutdown timeout, requests still pending', { count: pendingRequests.size }); - } else { - info('All requests completed'); - } - } - - // 2. Stop conversation cleanup timer + // 1. Stop conversation cleanup timer stopConversationCleanup(); - // 3. Save state after pending requests complete + // 2. Save state info('Saving conversation state'); saveState(); - // 4. Close database pool + // 3. Close database pool info('Closing database connection'); try { await closeDb(); @@ -256,11 +218,11 @@ async function gracefulShutdown(signal) { error('Failed to close database pool', { error: err.message }); } - // 5. Destroy Discord client + // 4. Destroy Discord client info('Disconnecting from Discord'); client.destroy(); - // 6. Log clean exit + // 5. Log clean exit info('Shutdown complete'); process.exit(0); } @@ -278,13 +240,6 @@ client.on('error', (err) => { }); }); -process.on('unhandledRejection', (err) => { - error('Unhandled promise rejection', { - error: err?.message || String(err), - stack: err?.stack, - type: typeof err, - }); -}); // Start bot const token = process.env.DISCORD_TOKEN; if (!token) { diff --git a/src/modules/chimeIn.js b/src/modules/chimeIn.js index f823e15c..f02c3a15 100644 --- a/src/modules/chimeIn.js +++ b/src/modules/chimeIn.js @@ -281,6 +281,7 @@ export async function accumulate(message, config) { // Reset counter so we don't spin on errors buf.counter = 0; } finally { + buf.abortController = null; evaluatingChannels.delete(channelId); } } diff --git a/src/modules/events.js b/src/modules/events.js index a570163c..9de7d358 100644 --- a/src/modules/events.js +++ b/src/modules/events.js @@ -5,6 +5,7 @@ import { Client, Events } from 'discord.js'; import { info, error as logError, warn } from '../logger.js'; +import { getUserFriendlyMessage } from '../utils/errors.js'; import { needsSplitting, splitMessage } from '../utils/splitMessage.js'; import { generateResponse } from './ai.js'; import { accumulate, resetCounter } from './chimeIn.js'; @@ -93,29 +94,42 @@ export function registerMessageCreateHandler(client, config, healthMonitor) { .replace(new RegExp(`<@!?${client.user.id}>`, 'g'), '') .trim(); - if (!cleanContent) { - await message.reply("Hey! What's up?"); - return; - } + try { + if (!cleanContent) { + await message.reply("Hey! What's up?"); + return; + } - await message.channel.sendTyping(); - - const response = await generateResponse( - message.channel.id, - cleanContent, - message.author.username, - config, - healthMonitor, - ); - - // Split long responses - if (needsSplitting(response)) { - const chunks = splitMessage(response); - for (const chunk of chunks) { - await message.channel.send(chunk); + await message.channel.sendTyping(); + + const response = await generateResponse( + message.channel.id, + cleanContent, + message.author.username, + config, + healthMonitor, + ); + + // Split long responses + if (needsSplitting(response)) { + const chunks = splitMessage(response); + for (const chunk of chunks) { + await message.channel.send(chunk); + } + } else { + await message.reply(response); + } + } catch (sendErr) { + logError('Failed to send AI response', { + channelId: message.channel.id, + error: sendErr.message, + }); + // Best-effort fallback — if the channel is still reachable, let the user know + try { + await message.reply(getUserFriendlyMessage(sendErr)); + } catch { + // Channel is unreachable — nothing more we can do } - } else { - await message.reply(response); } return; // Don't accumulate direct mentions into chime-in buffer diff --git a/src/modules/welcome.js b/src/modules/welcome.js index b383d41c..74c6b27a 100644 --- a/src/modules/welcome.js +++ b/src/modules/welcome.js @@ -8,6 +8,10 @@ import { info, error as logError } from '../logger.js'; const guildActivity = new Map(); const DEFAULT_ACTIVITY_WINDOW_MINUTES = 45; const MAX_EVENTS_PER_CHANNEL = 250; +const EVICTION_INTERVAL = 50; + +/** Counter for throttled eviction inside recordCommunityActivity */ +let activityCallCount = 0; /** Notable member-count milestones (hoisted to avoid allocation per welcome event) */ const NOTABLE_MILESTONES = new Set([10, 25, 50, 100, 250, 500, 1000]); @@ -68,6 +72,31 @@ export function recordCommunityActivity(message, config) { } activityMap.set(message.channel.id, timestamps); + + // Periodically prune stale channels to prevent unbounded memory growth + activityCallCount += 1; + if (activityCallCount >= EVICTION_INTERVAL) { + activityCallCount = 0; + pruneStaleActivity(cutoff); + } +} + +/** + * Prune channels with only stale timestamps from all guilds. + * @param {number} cutoff - Timestamp threshold; entries older than this are stale + */ +function pruneStaleActivity(cutoff) { + for (const [guildId, activityMap] of guildActivity) { + for (const [channelId, timestamps] of activityMap) { + // If the newest timestamp is older than the cutoff, the entire array is stale + if (!timestamps.length || timestamps[timestamps.length - 1] < cutoff) { + activityMap.delete(channelId); + } + } + if (activityMap.size === 0) { + guildActivity.delete(guildId); + } + } } /** diff --git a/tests/commands/config.test.js b/tests/commands/config.test.js index 69234cc6..b06b219c 100644 --- a/tests/commands/config.test.js +++ b/tests/commands/config.test.js @@ -169,6 +169,30 @@ describe('config command', () => { }), ); }); + + it('should hide raw error details when NODE_ENV is unset', async () => { + const originalEnv = process.env.NODE_ENV; + delete process.env.NODE_ENV; + + getConfig.mockImplementationOnce(() => { + throw new Error('pg: connection refused at 10.0.0.5:5432'); + }); + const mockReply = vi.fn(); + const interaction = { + options: { + getSubcommand: vi.fn().mockReturnValue('view'), + getString: vi.fn().mockReturnValue(null), + }, + reply: mockReply, + }; + + await execute(interaction); + const content = mockReply.mock.calls[0][0].content; + expect(content).toContain('An internal error occurred.'); + expect(content).not.toContain('pg: connection refused'); + + process.env.NODE_ENV = originalEnv; + }); }); describe('set subcommand', () => { diff --git a/tests/index.test.js b/tests/index.test.js index 5fed0c7f..0ecfb612 100644 --- a/tests/index.test.js +++ b/tests/index.test.js @@ -291,16 +291,6 @@ describe('index.js', () => { expect(mocks.ai.setConversationHistory).toHaveBeenCalled(); }); - it('should export pending request helpers', async () => { - const mod = await importIndex({ token: 'abc', databaseUrl: null }); - - const requestId = mod.registerPendingRequest(); - expect(typeof requestId).toBe('symbol'); - - // should not throw - mod.removePendingRequest(requestId); - }); - it('should handle autocomplete interactions', async () => { await importIndex({ token: 'abc', databaseUrl: null }); @@ -542,18 +532,6 @@ describe('index.js', () => { }); }); - it('should log unhandledRejection events', async () => { - await importIndex({ token: 'abc', databaseUrl: null }); - - mocks.processHandlers.unhandledRejection(new Error('rejected')); - - expect(mocks.logger.error).toHaveBeenCalledWith('Unhandled promise rejection', { - error: 'rejected', - stack: expect.any(String), - type: 'object', - }); - }); - it('should handle startup failure and exit', async () => { await importIndex({ token: 'abc', diff --git a/tests/modules/events.test.js b/tests/modules/events.test.js index ad9292a7..8fac352d 100644 --- a/tests/modules/events.test.js +++ b/tests/modules/events.test.js @@ -31,6 +31,11 @@ vi.mock('../../src/modules/welcome.js', () => ({ recordCommunityActivity: vi.fn(), })); +// Mock errors utility +vi.mock('../../src/utils/errors.js', () => ({ + getUserFriendlyMessage: vi.fn().mockReturnValue('Something went wrong. Try again!'), +})); + // Mock splitMessage vi.mock('../../src/utils/splitMessage.js', () => ({ needsSplitting: vi.fn().mockReturnValue(false), @@ -48,6 +53,7 @@ import { } from '../../src/modules/events.js'; import { isSpam, sendSpamAlert } from '../../src/modules/spam.js'; import { recordCommunityActivity, sendWelcomeMessage } from '../../src/modules/welcome.js'; +import { getUserFriendlyMessage } from '../../src/utils/errors.js'; import { needsSplitting, splitMessage } from '../../src/utils/splitMessage.js'; describe('events module', () => { @@ -238,6 +244,42 @@ describe('events module', () => { expect(mockSend).toHaveBeenCalledWith('chunk2'); }); + it('should handle message.reply() failure gracefully', async () => { + setup(); + const mockReply = vi.fn().mockRejectedValue(new Error('Missing Permissions')); + const message = { + author: { bot: false, username: 'user' }, + guild: { id: 'g1' }, + content: `<@bot-user-id> hello`, + channel: { id: 'c1', sendTyping: vi.fn().mockResolvedValue(undefined), send: vi.fn() }, + mentions: { has: vi.fn().mockReturnValue(true), repliedUser: null }, + reference: null, + reply: mockReply, + }; + await onCallbacks.messageCreate(message); + // Should not throw — error is caught and logged + expect(getUserFriendlyMessage).toHaveBeenCalled(); + }); + + it('should handle message.channel.send() failure during split gracefully', async () => { + setup(); + needsSplitting.mockReturnValueOnce(true); + splitMessage.mockReturnValueOnce(['chunk1', 'chunk2']); + const mockSend = vi.fn().mockRejectedValue(new Error('Unknown Channel')); + const mockReply = vi.fn().mockRejectedValue(new Error('Unknown Channel')); + const message = { + author: { bot: false, username: 'user' }, + guild: { id: 'g1' }, + content: `<@bot-user-id> tell me a story`, + channel: { id: 'c1', sendTyping: vi.fn().mockResolvedValue(undefined), send: mockSend }, + mentions: { has: vi.fn().mockReturnValue(true), repliedUser: null }, + reference: null, + reply: mockReply, + }; + await onCallbacks.messageCreate(message); + // Should not throw — error is caught and logged + }); + it('should respect allowed channels', async () => { setup({ ai: { enabled: true, channels: ['allowed-ch'] } }); const mockReply = vi.fn(); diff --git a/tests/modules/welcome.test.js b/tests/modules/welcome.test.js index bb9874a2..20ebeb05 100644 --- a/tests/modules/welcome.test.js +++ b/tests/modules/welcome.test.js @@ -135,6 +135,43 @@ describe('recordCommunityActivity', () => { }; recordCommunityActivity(message, {}); }); + + it('should prune stale activity data after enough calls', () => { + const config = { + welcome: { + dynamic: { activityWindowMinutes: 1 }, + }, + }; + + // Record activity in a channel + const msg = { + guild: { id: 'prune-guild' }, + channel: { id: 'prune-ch', isTextBased: () => true }, + author: { bot: false }, + }; + recordCommunityActivity(msg, config); + + // Fast-forward time past the activity window by making many calls + // with a fresh channel so the old one becomes stale. + // The eviction interval is 50, so we need at least 50 calls. + const now = Date.now(); + vi.spyOn(Date, 'now').mockReturnValue(now + 2 * 60 * 1000); // 2 minutes later + + const freshMsg = { + guild: { id: 'prune-guild' }, + channel: { id: 'fresh-ch', isTextBased: () => true }, + author: { bot: false }, + }; + for (let i = 0; i < 55; i++) { + recordCommunityActivity(freshMsg, config); + } + + vi.restoreAllMocks(); + + // Stale channel should have been pruned — calling recordCommunityActivity + // for the stale channel should create a fresh entry (no old timestamps) + // We can't inspect the Map directly, but at minimum it shouldn't crash + }); }); describe('sendWelcomeMessage', () => { From 7661eb662469a2f1b3ad20621d13498b6ffa7d13 Mon Sep 17 00:00:00 2001 From: AnExiledDev Date: Thu, 12 Feb 2026 03:46:23 +0000 Subject: [PATCH 2/3] fix(tests): restore NODE_ENV safely when original value is undefined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit process.env coerces undefined to the string "undefined" — use delete instead, wrapped in try/finally for robustness. --- tests/commands/config.test.js | 42 ++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/tests/commands/config.test.js b/tests/commands/config.test.js index b06b219c..eecc2b8f 100644 --- a/tests/commands/config.test.js +++ b/tests/commands/config.test.js @@ -174,24 +174,30 @@ describe('config command', () => { const originalEnv = process.env.NODE_ENV; delete process.env.NODE_ENV; - getConfig.mockImplementationOnce(() => { - throw new Error('pg: connection refused at 10.0.0.5:5432'); - }); - const mockReply = vi.fn(); - const interaction = { - options: { - getSubcommand: vi.fn().mockReturnValue('view'), - getString: vi.fn().mockReturnValue(null), - }, - reply: mockReply, - }; - - await execute(interaction); - const content = mockReply.mock.calls[0][0].content; - expect(content).toContain('An internal error occurred.'); - expect(content).not.toContain('pg: connection refused'); - - process.env.NODE_ENV = originalEnv; + try { + getConfig.mockImplementationOnce(() => { + throw new Error('pg: connection refused at 10.0.0.5:5432'); + }); + const mockReply = vi.fn(); + const interaction = { + options: { + getSubcommand: vi.fn().mockReturnValue('view'), + getString: vi.fn().mockReturnValue(null), + }, + reply: mockReply, + }; + + await execute(interaction); + const content = mockReply.mock.calls[0][0].content; + expect(content).toContain('An internal error occurred.'); + expect(content).not.toContain('pg: connection refused'); + } finally { + if (originalEnv === undefined) { + delete process.env.NODE_ENV; + } else { + process.env.NODE_ENV = originalEnv; + } + } }); }); From 68e16cfd9d5f9506af3ba173285464b84e8aee74 Mon Sep 17 00:00:00 2001 From: Pip Build Date: Wed, 11 Feb 2026 22:53:09 -0500 Subject: [PATCH 3/3] test: assert welcome activity pruning deterministically --- src/modules/welcome.js | 23 ++++++++++++++++++++ tests/modules/welcome.test.js | 40 +++++++++++++++++++++++------------ 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/src/modules/welcome.js b/src/modules/welcome.js index 74c6b27a..a6fe08d6 100644 --- a/src/modules/welcome.js +++ b/src/modules/welcome.js @@ -19,6 +19,29 @@ const NOTABLE_MILESTONES = new Set([10, 25, 50, 100, 250, 500, 1000]); /** @type {{key: string, set: Set} | null} Cached excluded channels Set */ let excludedChannelsCache = null; +/** + * Test-only helper: snapshot guild activity state. + * @param {string} guildId - Guild ID + * @returns {Record} + */ +export function __getCommunityActivityState(guildId) { + const activityMap = guildActivity.get(guildId); + if (!activityMap) return {}; + + return Object.fromEntries( + [...activityMap.entries()].map(([channelId, timestamps]) => [channelId, [...timestamps]]), + ); +} + +/** + * Test-only helper: clear in-memory activity state. + */ +export function __resetCommunityActivityState() { + guildActivity.clear(); + activityCallCount = 0; + excludedChannelsCache = null; +} + /** * Render welcome message with placeholder replacements * @param {string} messageTemplate - Welcome message template diff --git a/tests/modules/welcome.test.js b/tests/modules/welcome.test.js index 20ebeb05..fccf86bb 100644 --- a/tests/modules/welcome.test.js +++ b/tests/modules/welcome.test.js @@ -9,6 +9,8 @@ vi.mock('../../src/logger.js', () => ({ })); import { + __getCommunityActivityState, + __resetCommunityActivityState, recordCommunityActivity, renderWelcomeMessage, sendWelcomeMessage, @@ -137,40 +139,52 @@ describe('recordCommunityActivity', () => { }); it('should prune stale activity data after enough calls', () => { + __resetCommunityActivityState(); + const config = { welcome: { - dynamic: { activityWindowMinutes: 1 }, + dynamic: { activityWindowMinutes: 5 }, }, }; - // Record activity in a channel - const msg = { + const baseTime = 1_700_000_000_000; + const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(baseTime); + + // Record activity in a channel that will become stale + const staleMsg = { guild: { id: 'prune-guild' }, channel: { id: 'prune-ch', isTextBased: () => true }, author: { bot: false }, }; - recordCommunityActivity(msg, config); + recordCommunityActivity(staleMsg, config); - // Fast-forward time past the activity window by making many calls - // with a fresh channel so the old one becomes stale. - // The eviction interval is 50, so we need at least 50 calls. - const now = Date.now(); - vi.spyOn(Date, 'now').mockReturnValue(now + 2 * 60 * 1000); // 2 minutes later + expect(__getCommunityActivityState('prune-guild')).toEqual({ + 'prune-ch': [baseTime], + }); + + // Fast-forward time past the activity window and trigger periodic eviction. + // The eviction interval is 50, so we exceed it. + nowSpy.mockReturnValue(baseTime + 10 * 60 * 1000); const freshMsg = { guild: { id: 'prune-guild' }, channel: { id: 'fresh-ch', isTextBased: () => true }, author: { bot: false }, }; + for (let i = 0; i < 55; i++) { recordCommunityActivity(freshMsg, config); } - vi.restoreAllMocks(); + const state = __getCommunityActivityState('prune-guild'); + + // Prove stale channel was evicted and only fresh channel timestamps remain. + expect(state['prune-ch']).toBeUndefined(); + expect(state['fresh-ch']).toHaveLength(55); + expect(state['fresh-ch']).toEqual(Array(55).fill(baseTime + 10 * 60 * 1000)); - // Stale channel should have been pruned — calling recordCommunityActivity - // for the stale channel should create a fresh entry (no old timestamps) - // We can't inspect the Map directly, but at minimum it shouldn't crash + vi.restoreAllMocks(); + __resetCommunityActivityState(); }); });