audit week 3 quality batch: QUAL-004/005/007/008/010 + SEC-002

QUAL-004 handlers/messages.js — DM-on-customer-reply now reads
guild.members.cache.get(claimerId) first and only falls back to
guild.members.fetch on cache miss. Avoids a REST round-trip per non-staff
reply on busy tickets. GuildMembers intent already keeps the cache warm.

QUAL-005 handlers/buttons.js (runFinalClose) + handlers/commands/close.js
(finalizeForceClose) — close paths now $unset welcomeMessageId alongside
the status: 'closed' write. Stops a stale message-ID from carrying into a
future reopen on the same Gmail thread, where escalation's "edit welcome
buttons" path would silently fail trying to fetch a message in a deleted
channel.

QUAL-007 services/configPersistence.js — writeEnvFile mismatch error now
includes the missing/extra key sets, not just count vs count. Saves the
operator from guessing which key vanished after a partial write.

QUAL-008 utils.js stripEmailQuotes — replaced order-dependent first-match
loop with an earliest-match-across-all-markers scan. The previous code
could truncate at a late "_____" signature underline even when an earlier
"On X wrote:" reply header was the real cutoff. New test in
tests/utils.test.js exercises the dual-marker case.

QUAL-010 broccolini-discord.js — moved `let httpServer / internalServer /
appReady` declarations from after the ready handler to before it. Same
runtime behavior (module-load completes before ready fires asynchronously),
but the read order now matches the assignment order.

SEC-002 routes/internalApi.js — POST /restart now goes through a tighter
2/min limiter on top of the shared 10/min internalLimiter. Defense in
depth in case INTERNAL_API_SECRET ever leaks; an attacker with the secret
can no longer crash-loop the container.

Skipped: QUAL-009 (re-checked the regex; ^\s*\n* → \n is already
idempotent — the audit finding was incorrect).

vitest run: 88/88 (one new test for QUAL-008).

broccolini-discord.js

@@ -165,6 +165,14 @@ client.on('messageCreate', async msg => {
   await handleDiscordReply(msg);
 });
 
+// HTTP server handles + readiness flag. Assigned inside the ready callback
+// (httpServer, appReady) and the INTERNAL_API_SECRET branch below
+// (internalServer); declared here so they're visible to the ready callback,
+// the express middleware below, and the shutdown handler at the bottom.
+let httpServer = null;
+let internalServer = null;
+let appReady = false;
+
 client.once('ready', async () => {
   if (!process.env.MONGODB_URI) {
     console.error('MONGODB_URI is not set in .env. Broccolini Bot requires MongoDB.');
@@ -228,7 +236,7 @@ client.login(CONFIG.DISCORD_TOKEN);
 const app = express();
 app.use(express.json());
 // Reject API traffic with 503 until ready event has fired and routes are mounted.
-let appReady = false;
+// (appReady is declared at module top so the ready callback can flip it.)
 app.use((req, res, next) => {
   if (!appReady && req.path.startsWith('/api')) {
     return res.status(503).json({ error: 'Bot is starting; API not ready yet.' });
@@ -243,8 +251,6 @@ const internalApi = require('./routes/internalApi');
 const internalApp = express();
 internalApp.use('/internal', internalApi);
 
-let httpServer = null;
-let internalServer = null;
 if (CONFIG.INTERNAL_API_SECRET) {
   // Must bind all-interfaces inside the bot container: the settings-site is a
   // separate container on broccoli-net and reaches this API over the docker

handlers/buttons.js

@@ -442,9 +442,11 @@ async function runFinalClose(interaction, ticket, sendEmail = true) {
       await sendTicketClosedEmail(ticket, closerDisplayName, interaction.user.id);
     }
 
+    // $unset welcomeMessageId so a future reopen on this thread doesn't carry
+    // a stale message ID pointing into the now-deleted channel.
     await Ticket.updateOne(
       { gmailThreadId: ticket.gmailThreadId },
-      { $set: { discordThreadId: null, status: 'closed' } }
+      { $set: { discordThreadId: null, status: 'closed' }, $unset: { welcomeMessageId: '' } }
     );
 
     if (transcriptMsg?.id) {

handlers/commands/close.js

@@ -66,9 +66,11 @@ async function finalizeForceClose(channelRef, clientRef) {
   if (!freshTicket || freshTicket.status === 'closed') return;
 
   try {
+    // $unset welcomeMessageId so a future reopen on this thread doesn't carry
+    // a stale message ID pointing into the now-deleted channel.
     await Ticket.updateOne(
       { gmailThreadId: freshTicket.gmailThreadId },
-      { $set: { status: 'closed' } }
+      { $set: { status: 'closed' }, $unset: { welcomeMessageId: '' } }
     );
 
     await enqueueSend(channelRef, 'Ticket force-closed. Archiving...');

handlers/messages.js

@@ -31,7 +31,11 @@ async function handleDiscordReply(m) {
   if (ticket.claimerId && !isStaffMember && m.author.id !== ticket.claimerId) {
     const dmEnabled = await getNotifyDm(ticket.claimerId);
     if (dmEnabled) {
-      const staffMember = await m.guild.members.fetch(ticket.claimerId).catch(() => null);
+      // Cache-first: GuildMembers intent keeps the cache populated; only fetch
+      // on miss (e.g. cold cache after restart). Avoids a REST round-trip on
+      // every customer reply in a busy ticket.
+      const staffMember = m.guild.members.cache.get(ticket.claimerId)
+        || await m.guild.members.fetch(ticket.claimerId).catch(() => null);
       if (staffMember) {
         const jumpLink = `https://discord.com/channels/${m.guild.id}/${m.channel.id}/${m.id}`;
         await staffMember

routes/internalApi.js

@@ -19,6 +19,17 @@ const internalLimiter = rateLimit({
   message: { error: 'Too many requests, please try again later.' }
 });
 
+// /restart calls process.exit; defense-in-depth tighter floor in case the
+// shared INTERNAL_API_SECRET ever leaks. 2/min is enough for an operator-
+// driven retry but not enough to crash-loop the container.
+const restartLimiter = rateLimit({
+  windowMs: 60 * 1000,
+  max: 2,
+  standardHeaders: true,
+  legacyHeaders: false,
+  message: { error: 'Too many restart attempts.' }
+});
+
 router.use(internalLimiter);
 
 // Middleware: verify internal secret
@@ -111,7 +122,7 @@ router.get('/discord/guild', async (req, res) => {
 // POST /restart — restart the bot process
 let scheduledRestart = null;
 
-router.post('/restart', express.json(), (req, res) => {
+router.post('/restart', restartLimiter, express.json(), (req, res) => {
   const { mode, scheduledFor } = req.body;
 
   if (mode === 'immediate') {

services/configPersistence.js

@@ -150,7 +150,16 @@ function writeEnvFile(updates) {
 
   const roundtrip = readEnvFile();
   if (roundtrip.size !== expected) {
-    throw new Error(`writeEnvFile: key count mismatch after write (expected ${expected}, got ${roundtrip.size})`);
+    const expectedKeys = new Set(updates.keys());
+    const actualKeys = new Set(roundtrip.keys());
+    const missing = [...expectedKeys].filter(k => !actualKeys.has(k));
+    const extra = [...actualKeys].filter(k => !expectedKeys.has(k));
+    throw new Error(
+      `writeEnvFile: key count mismatch after write ` +
+      `(expected ${expected}, got ${roundtrip.size})` +
+      (missing.length ? `. Missing: [${missing.join(', ')}]` : '') +
+      (extra.length ? `. Extra: [${extra.join(', ')}]` : '')
+    );
   }
 }
 

tests/utils.test.js

@@ -44,6 +44,13 @@ describe('stripEmailQuotes', () => {
     const input = 'New reply.\r\nOn Monday Bob <b@x.com> wrote:\r\n> quoted';
     expect(stripEmailQuotes(input)).toBe('New reply.');
   });
+
+  it('picks earliest cutoff when multiple markers match', () => {
+    // Earlier in the body: "On X wrote:". Later: "_____" underline.
+    // The earliest cutoff is the reply marker, not the underline.
+    const input = 'My new reply.\nOn Mon Bob wrote:\n> quoted text\n_____\nsignature';
+    expect(stripEmailQuotes(input)).toBe('My new reply.');
+  });
 });
 
 describe('stripMobileFooter', () => {

utils.js

@@ -112,22 +112,29 @@ function getCleanBody(payload) {
 function stripEmailQuotes(text) {
   let cleaned = text.replace(/\r\n/g, '\n');
 
+  // Pick the earliest match across all markers, not just the first marker that
+  // matches anywhere. The previous order-dependent loop could truncate at a
+  // late "_____" signature underline even when an earlier "On X wrote:" reply
+  // header was the real cutoff.
   const markers = [
-    /\n_{5,}\s*$/m,
+    /\nOn .* wrote:/i,
     /\nFrom:\s.*<.*@.*>/i,
     /\nSent:\s.*$/i,
     /\nTo:\s.*$/i,
     /\nSubject:\s.*$/i,
-    /\nOn .* wrote:/i
+    /\n_{5,}\s*$/m
   ];
 
+  let earliest = -1;
   for (const m of markers) {
     const match = cleaned.match(m);
-    if (match) {
+    if (match && (earliest === -1 || match.index < earliest)) {
-      cleaned = cleaned.substring(0, match.index);
+      earliest = match.index;
-      break;
     }
   }
+  if (earliest !== -1) {
+    cleaned = cleaned.substring(0, earliest);
+  }
 
   return cleaned.trim();
 }