refactor(backup): convert sync fs operations to async promises

CODE_IMPROVEMENTS.md

@@ -0,0 +1,176 @@
+# Code Improvement Opportunities
+
+Based on comprehensive analysis of the volvox-bot codebase (159 JS files, ~50k lines).
+
+## 🔴 Critical Issues
+
+### 1. Large File Refactoring Needed
+**Files exceeding 500 lines need decomposition:**
+
+| File | Lines | Issue |
+|------|-------|-------|
+| `src/api/routes/guilds.js` | 1,622 | God route - handles analytics, members, config, moderation |
+| `src/api/routes/conversations.js` | 1,069 | Multiple concerns (conversations + flagged messages) |
+| `src/api/routes/members.js` | 1,006 | Member management + bulk actions + reputation |
+| `src/modules/events.js` | 959 | Event handler doing too much - violates SRP |
+| `src/modules/config.js` | 904 | Config logic + caching + merging + validation |
+| `src/modules/triage.js` | 806 | Complex AI triage - could split by stage |
+
+**Recommendation:** Split into smaller modules with single responsibilities.
+
+---
+
+## 🟠 High Priority
+
+### 2. Missing Test Coverage
+**Files without tests:**
+- `src/modules/pollHandler.js` - No tests
+- `src/modules/reputationDefaults.js` - No tests
+- `src/modules/reviewHandler.js` - No tests
+- `src/utils/cronParser.js` - No tests
+- `src/utils/flattenToLeafPaths.js` - No tests
+- `src/commands/voice.js` - Command exists but no test file
+
+### 3. TODO Items in Code
+```javascript
+// src/utils/permissions.js:132
+// TODO(#71): check guild-scoped admin roles once per-guild config is implemented
+
+// src/api/routes/guilds.js:1182
+// TODO(issue-122): move slash-command analytics to a dedicated usage table
+
+// src/modules/backup.js:18
+// TODO: Consider switching to fs.promises for async operations
+```
+
+### 4. Magic Numbers & Hardcoded Values
+Many time constants scattered throughout:
+```javascript
+// Should be configurable:
+24 * 60 * 60 * 1000        // 1 day - appears 8+ times
+15 * 60 * 1000             // 15 min - rate limit windows
+365 * 24 * 60 * 60 * 1000  // 1 year - max duration
+MAX_MEMORY_CACHE_SIZE = 1000  // utils/cache.js
+```
+
+**Recommendation:** Centralize in `src/constants/time.js` or make configurable.
+
+---
+
+## 🟡 Medium Priority
+
+### 5. Error Handling Inconsistencies
+
+**Inconsistent catch patterns:**
+```javascript
+// Some use empty catch (swallow errors):
+} catch {
+  // nothing
+}
+
+// Some log but don't rethrow:
+} catch (err) {
+  logError('...', { error: err.message });
+}
+
+// Some properly handle:
+} catch (err) {
+  error('Failed to...', { error: err.message });
+  throw err;
+}
+```
+
+**Files with empty catches to review:**
+- `src/utils/cache.js:87`
+- `src/utils/guildSpend.js:28`
+- `src/utils/debugFooter.js:298`
+- `src/api/utils/sessionStore.js:71`
+- `src/api/utils/webhook.js:24`
+- `src/api/utils/ssrfProtection.js:204,266`
+- `src/api/middleware/redisRateLimit.js:69`
+- `src/api/middleware/auditLog.js:140,203,231`
+- `src/api/middleware/verifyJwt.js:46,53`
+- `src/api/routes/community.js:714`
+- `src/api/routes/health.js:20`
+
+### 6. Potential Memory Leaks
+
+**Event listeners without removal:**
+- 58 `.on()` / `.once()` calls found
+- Need audit of listener cleanup on shutdown/restart
+
+**Timers without cleanup:**
+- 55 `setTimeout` / `setInterval` instances
+- Some may not be cleared on error paths
+
+### 7. Database Query Patterns
+
+**Good:** All queries use parameterized inputs (no SQL injection risk)
+
+**Could improve:**
+- Some queries build dynamic WHERE clauses - verify all are safe
+- Missing query timeout handling in some places
+- No connection pool exhaustion handling visible
+
+---
+
+## 🟢 Low Priority / Polish
+
+### 8. Code Organization
+
+**Import ordering inconsistent:**
+Some files group by type (builtins, external, internal), others don't.
+
+**Example standard:**
+```javascript
+// 1. Node builtins
+import { readFileSync } from 'node:fs';
+
+// 2. External packages
+import { Client } from 'discord.js';
+
+// 3. Internal modules (absolute)
+import { getConfig } from '../modules/config.js';
+
+// 4. Internal modules (relative)
+import { helper } from './helper.js';
+```
+
+### 9. JSDoc Coverage
+
+Many functions lack JSDoc types, making IDE support weaker.
+
+### 10. Naming Consistency
+
+Some inconsistency in naming:
+- `logError` vs `error` (logger imports)
+- `guildId` vs `id` vs `serverId` in different contexts
+- `userId` vs `user_id` (JS vs DB naming)
+
+---
+
+## 📊 Metrics Summary
+
+| Metric | Count |
+|--------|-------|
+| Total JS files | 159 |
+| Async functions | 441 |
+| Await statements | 1,334 |
+| Promise chains (.then/.catch) | 149 |
+| Throw statements | 90 |
+| New Error instances | 52 |
+| Database queries | 816 |
+| setTimeout/setInterval | 55 |
+| Event listeners | 58 |
+| TODO/FIXME comments | 3 |
+
+---
+
+## 🎯 Recommended Actions (Priority Order)
+
+1. **Split large route files** — Start with `guilds.js` (1,622 lines)
+2. **Add missing tests** — Focus on `pollHandler.js`, `reviewHandler.js`
+3. **Centralize magic numbers** — Create `src/constants/` directory
+4. **Audit error handling** — Review all empty catch blocks
+5. **Document public APIs** — Add JSDoc to exported functions
+6. **Standardize imports** — Enforce consistent ordering via lint rule

TASK.md

@@ -0,0 +1,74 @@
+# Task: Centralize Magic Numbers and Time Constants
+
+## Context
+The codebase has time constants scattered throughout (24 * 60 * 60 * 1000 appearing 8+ times). This makes maintenance hard and configuration impossible.
+
+## Files to Work On
+- Create: `src/constants/time.js` - Time duration constants
+- Create: `src/constants/index.js` - Re-export all constants
+- Update files that use magic numbers:
+  - `src/utils/duration.js`
+  - `src/utils/guildSpend.js`
+  - `src/utils/cache.js`
+  - `src/api/middleware/rateLimit.js`
+  - `src/api/middleware/redisRateLimit.js`
+  - And others identified in CODE_IMPROVEMENTS.md
+
+## Requirements
+
+### Phase 1: Create Constants Module
+Create `src/constants/time.js` with:
+```javascript
+export const MS_PER_SECOND = 1000;
+export const MS_PER_MINUTE = 60 * 1000;
+export const MS_PER_HOUR = 60 * 60 * 1000;
+export const MS_PER_DAY = 24 * 60 * 60 * 1000;
+export const MS_PER_WEEK = 7 * 24 * 60 * 60 * 1000;
+export const MS_PER_YEAR = 365 * 24 * 60 * 60 * 1000;
+
+// Common durations
+export const DURATION = {
+  MINUTE: MS_PER_MINUTE,
+  HOUR: MS_PER_HOUR,
+  DAY: MS_PER_DAY,
+  WEEK: MS_PER_WEEK,
+  YEAR: MS_PER_YEAR,
+};
+
+// Rate limit windows
+export const RATE_LIMIT = {
+  SHORT: 15 * MS_PER_MINUTE,  // 15 minutes
+  MEDIUM: MS_PER_HOUR,
+  LONG: MS_PER_DAY,
+};
+```
+
+### Phase 2: Replace Magic Numbers
+Replace inline calculations with imports:
+- `24 * 60 * 60 * 1000` → `MS_PER_DAY` or `DURATION.DAY`
+- `15 * 60 * 1000` → `RATE_LIMIT.SHORT`
+- etc.
+
+### Phase 3: Configurable Cache Sizes
+Move hardcoded limits to constants:
+- `MAX_MEMORY_CACHE_SIZE = 1000` in cache.js
+- `MAX_ANALYTICS_RANGE_DAYS = 90` in guilds.js
+
+## Constraints
+- Do NOT change any behavior - only replace constants
+- Keep all tests passing
+- Run lint after changes
+
+## Acceptance Criteria
+- [ ] src/constants/time.js created with all time constants
+- [ ] src/constants/index.js created for clean imports
+- [ ] All magic number occurrences replaced
+- [ ] No behavioral changes
+- [ ] All tests pass
+- [ ] Lint passes
+
+## Progress Tracking
+Commit after each file is updated:
+1. "refactor: create centralized time constants module"
+2. "refactor: replace magic numbers in duration.js"
+3. etc.

src/api/middleware/rateLimit.js

@@ -3,6 +3,8 @@
  * Simple in-memory per-IP rate limiter with no external dependencies
  */
 
+import { RATE_LIMIT_WINDOW } from '../../constants/index.js';
+
 const DEFAULT_MESSAGE = 'Too many requests, please try again later';
 
 /**
@@ -16,7 +18,7 @@ const DEFAULT_MESSAGE = 'Too many requests, please try again later';
  * @returns {import('express').RequestHandler & { destroy: () => void }} Express middleware with a destroy method to clear the cleanup timer
  */
 export function rateLimit({
-  windowMs = 15 * 60 * 1000,
+  windowMs = RATE_LIMIT_WINDOW.SHORT,
   max = 100,
   message = DEFAULT_MESSAGE,
 } = {}) {

src/api/middleware/redisRateLimit.js

@@ -6,6 +6,7 @@
  * @see https://github.com/VolvoxLLC/volvox-bot/issues/177
  */
 
+import { RATE_LIMIT_WINDOW } from '../../constants/index.js';
 import { getRedis } from '../../redis.js';
 import { rateLimit as inMemoryRateLimit } from './rateLimit.js';
 
@@ -19,7 +20,11 @@ import { rateLimit as inMemoryRateLimit } from './rateLimit.js';
  * @param {string} [options.keyPrefix='rl'] - Redis key prefix
  * @returns {import('express').RequestHandler & { destroy: () => void }}
  */
-export function redisRateLimit({ windowMs = 15 * 60 * 1000, max = 100, keyPrefix = 'rl' } = {}) {
+export function redisRateLimit({
+  windowMs = RATE_LIMIT_WINDOW.SHORT,
+  max = 100,
+  keyPrefix = 'rl',
+} = {}) {
   // Create in-memory fallback (always available)
   const fallback = inMemoryRateLimit({ windowMs, max });
 

src/api/routes/backup.js

@@ -184,8 +184,8 @@ router.post(
 router.get(
   '/',
   (req, res, next) => requireGlobalAdmin('Backup access', req, res, next),
-  (_req, res) => {
-    const backups = listBackups();
+  async (_req, res) => {
+    const backups = await listBackups();
     res.json(backups);
   },
 );
@@ -229,9 +229,9 @@ router.get(
 router.post(
   '/',
   (req, res, next) => requireGlobalAdmin('Backup access', req, res, next),
-  (_req, res) => {
+  async (_req, res) => {
     try {
-      const meta = createBackup();
+      const meta = await createBackup();
       return res.status(201).json({ id: meta.id, size: meta.size, createdAt: meta.createdAt });
     } catch (err) {
       return res.status(500).json({ error: 'Failed to create backup', details: err.message });
@@ -278,11 +278,11 @@ router.post(
 router.get(
   '/:id/download',
   (req, res, next) => requireGlobalAdmin('Backup access', req, res, next),
-  (req, res) => {
+  async (req, res) => {
     const { id } = req.params;
 
     try {
-      const payload = readBackup(id);
+      const payload = await readBackup(id);
       const filename = `${id}.json`;
       res.setHeader('Content-Disposition', `attachment; filename="${filename}"`);
       res.setHeader('Content-Type', 'application/json');
@@ -420,7 +420,7 @@ router.post(
 router.post(
   '/prune',
   (req, res, next) => requireGlobalAdmin('Backup access', req, res, next),
-  (req, res) => {
+  async (req, res) => {
     const retention = req.body ?? {};
     const errors = [];
 
@@ -439,7 +439,7 @@ router.post(
       return res.status(400).json({ error: 'Invalid prune options', details: errors });
     }
 
-    const deleted = pruneBackups(retention);
+    const deleted = await pruneBackups(retention);
     return res.json({ deleted, count: deleted.length });
   },
 );

src/constants/index.js

@@ -0,0 +1,15 @@
+/**
+ * Constants Module
+ * Re-exports all centralized constants for clean imports.
+ */
+
+export {
+  DURATION,
+  MS_PER_DAY,
+  MS_PER_HOUR,
+  MS_PER_MINUTE,
+  MS_PER_SECOND,
+  MS_PER_WEEK,
+  MS_PER_YEAR,
+  RATE_LIMIT_WINDOW,
+} from './time.js';

src/constants/time.js

@@ -0,0 +1,33 @@
+/**
+ * Time Duration Constants
+ * Centralized time constants to eliminate magic numbers throughout the codebase.
+ */
+
+// Base units in milliseconds
+export const MS_PER_SECOND = 1000;
+export const MS_PER_MINUTE = 60 * MS_PER_SECOND;
+export const MS_PER_HOUR = 60 * MS_PER_MINUTE;
+export const MS_PER_DAY = 24 * MS_PER_HOUR;
+export const MS_PER_WEEK = 7 * MS_PER_DAY;
+export const MS_PER_YEAR = 365 * MS_PER_DAY;
+
+/**
+ * Common duration values for convenience
+ */
+export const DURATION = {
+  SECOND: MS_PER_SECOND,
+  MINUTE: MS_PER_MINUTE,
+  HOUR: MS_PER_HOUR,
+  DAY: MS_PER_DAY,
+  WEEK: MS_PER_WEEK,
+  YEAR: MS_PER_YEAR,
+};
+
+/**
+ * Rate limit window presets
+ */
+export const RATE_LIMIT_WINDOW = {
+  SHORT: 15 * MS_PER_MINUTE, // 15 minutes
+  MEDIUM: MS_PER_HOUR, // 1 hour
+  LONG: MS_PER_DAY, // 24 hours
+};