diff --git a/CODE_IMPROVEMENTS.md b/CODE_IMPROVEMENTS.md new file mode 100644 index 00000000..c8e2996e --- /dev/null +++ b/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 diff --git a/TASK.md b/TASK.md new file mode 100644 index 00000000..bfd5dc12 --- /dev/null +++ b/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. diff --git a/src/api/middleware/rateLimit.js b/src/api/middleware/rateLimit.js index 8b1b988f..22d6b9b7 100644 --- a/src/api/middleware/rateLimit.js +++ b/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, } = {}) { diff --git a/src/api/middleware/redisRateLimit.js b/src/api/middleware/redisRateLimit.js index 846c8de3..f5ea51ab 100644 --- a/src/api/middleware/redisRateLimit.js +++ b/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 }); diff --git a/src/api/routes/backup.js b/src/api/routes/backup.js index cd2e2391..ad39cf4d 100644 --- a/src/api/routes/backup.js +++ b/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 }); }, ); diff --git a/src/constants/index.js b/src/constants/index.js new file mode 100644 index 00000000..dbf5e1cd --- /dev/null +++ b/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'; diff --git a/src/constants/time.js b/src/constants/time.js new file mode 100644 index 00000000..d8106444 --- /dev/null +++ b/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 +}; diff --git a/src/modules/backup.js b/src/modules/backup.js index 981aae20..ff03c8fc 100644 --- a/src/modules/backup.js +++ b/src/modules/backup.js @@ -5,18 +5,7 @@ * @see https://github.com/VolvoxLLC/volvox-bot/issues/129 */ -import { - existsSync, - mkdirSync, - readdirSync, - readFileSync, - statSync, - unlinkSync, - writeFileSync, -} from 'node:fs'; - -// TODO: Consider switching to fs.promises for async operations to improve performance -// and avoid blocking the event loop with synchronous file system operations. +import { access, constants, mkdir, readdir, readFile, stat, unlink, writeFile } from 'node:fs/promises'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { SAFE_CONFIG_KEYS, SENSITIVE_FIELDS } from '../api/utils/configAllowlist.js'; @@ -45,12 +34,14 @@ let scheduledBackupInterval = null; * Get or create the backup directory. * * @param {string} [dir] - Override backup directory path - * @returns {string} The backup directory path + * @returns {Promise} The backup directory path */ -export function getBackupDir(dir) { +export async function getBackupDir(dir) { const backupDir = dir ?? DEFAULT_BACKUP_DIR; - if (!existsSync(backupDir)) { - mkdirSync(backupDir, { recursive: true }); + try { + await access(backupDir, constants.F_OK); + } catch { + await mkdir(backupDir, { recursive: true }); } return backupDir; } @@ -192,10 +183,10 @@ function makeBackupFilename(date = new Date()) { * Create a timestamped backup of the current config in the backup directory. * * @param {string} [backupDir] - Override backup directory - * @returns {{id: string, path: string, size: number, createdAt: string}} Backup metadata + * @returns {Promise<{id: string, path: string, size: number, createdAt: string}>} Backup metadata */ -export function createBackup(backupDir) { - const dir = getBackupDir(backupDir); +export async function createBackup(backupDir) { + const dir = await getBackupDir(backupDir); const now = new Date(); const filename = makeBackupFilename(now); const filePath = path.join(dir, filename); @@ -203,17 +194,17 @@ export function createBackup(backupDir) { const payload = exportConfig(); const json = JSON.stringify(payload, null, 2); - writeFileSync(filePath, json, 'utf8'); + await writeFile(filePath, json, 'utf8'); - const { size } = statSync(filePath); + const stats = await stat(filePath); const id = filename.replace('.json', ''); - info('Config backup created', { id, path: filePath, size }); + info('Config backup created', { id, path: filePath, size: stats.size }); return { id, path: filePath, - size, + size: stats.size, createdAt: now.toISOString(), }; } @@ -223,16 +214,17 @@ export function createBackup(backupDir) { * * @param {string} filename - Backup filename * @param {string} dir - Directory containing the backup file - * @returns {{id: string, filename: string, createdAt: string, size: number} | null} + * @returns {Promise<{id: string, filename: string, createdAt: string, size: number} | null>} */ -function parseBackupMeta(filename, dir) { +async function parseBackupMeta(filename, dir) { const match = BACKUP_FILENAME_PATTERN.exec(filename); if (!match) return null; const filePath = path.join(dir, filename); let size = 0; try { - size = statSync(filePath).size; + const stats = await stat(filePath); + size = stats.size; } catch { return null; } @@ -252,20 +244,22 @@ function parseBackupMeta(filename, dir) { * List all available backups, sorted newest first. * * @param {string} [backupDir] - Override backup directory - * @returns {Array<{id: string, filename: string, createdAt: string, size: number}>} + * @returns {Promise>} */ -export function listBackups(backupDir) { - const dir = getBackupDir(backupDir); +export async function listBackups(backupDir) { + const dir = await getBackupDir(backupDir); let files; try { - files = readdirSync(dir); + files = await readdir(dir); } catch { return []; } - const backups = files - .map((filename) => parseBackupMeta(filename, dir)) + const backupMetaPromises = files.map((filename) => parseBackupMeta(filename, dir)); + const results = await Promise.all(backupMetaPromises); + + const backups = results .filter(Boolean) .sort((a, b) => new Date(b.createdAt) - new Date(a.createdAt)); @@ -277,10 +271,10 @@ export function listBackups(backupDir) { * * @param {string} id - Backup ID (filename without .json) * @param {string} [backupDir] - Override backup directory - * @returns {Object} Parsed backup payload + * @returns {Promise} Parsed backup payload * @throws {Error} If backup file not found or invalid */ -export function readBackup(id, backupDir) { +export async function readBackup(id, backupDir) { // Validate ID against strict pattern: backup-YYYY-MM-DDTHH-mm-ss-SSS-NNNN const BACKUP_ID_PATTERN = /^backup-[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}-[0-9]{2}-[0-9]{2}-[0-9]{3}-[0-9]{4}$/; @@ -288,15 +282,17 @@ export function readBackup(id, backupDir) { throw new Error('Invalid backup ID'); } - const dir = getBackupDir(backupDir); + const dir = await getBackupDir(backupDir); const filename = `${id}.json`; const filePath = path.join(dir, filename); - if (!existsSync(filePath)) { + try { + await access(filePath, constants.F_OK); + } catch { throw new Error(`Backup not found: ${id}`); } - const raw = readFileSync(filePath, 'utf8'); + const raw = await readFile(filePath, 'utf8'); try { return JSON.parse(raw); } catch { @@ -313,7 +309,7 @@ export function readBackup(id, backupDir) { * @throws {Error} If backup not found or invalid */ export async function restoreBackup(id, backupDir) { - const payload = readBackup(id, backupDir); + const payload = await readBackup(id, backupDir); const validationErrors = validateImportPayload(payload); if (validationErrors.length > 0) { @@ -338,12 +334,12 @@ export async function restoreBackup(id, backupDir) { * * @param {{daily?: number, weekly?: number}} [retention] - Retention counts * @param {string} [backupDir] - Override backup directory - * @returns {string[]} IDs of deleted backups + * @returns {Promise} IDs of deleted backups */ -export function pruneBackups(retention, backupDir) { +export async function pruneBackups(retention, backupDir) { const { daily = DEFAULT_RETENTION.daily, weekly = DEFAULT_RETENTION.weekly } = retention ?? {}; - const dir = getBackupDir(backupDir); - const all = listBackups(dir); + const dir = await getBackupDir(backupDir); + const all = await listBackups(dir); if (all.length === 0) return []; @@ -372,7 +368,7 @@ export function pruneBackups(retention, backupDir) { for (const backup of all) { if (!toKeep.has(backup.id)) { try { - unlinkSync(path.join(dir, backup.filename)); + await unlink(path.join(dir, backup.filename)); deleted.push(backup.id); info('Pruned old backup', { id: backup.id }); } catch (err) { diff --git a/src/utils/cache.js b/src/utils/cache.js index 1c8fa3bf..23109ec4 100644 --- a/src/utils/cache.js +++ b/src/utils/cache.js @@ -9,6 +9,7 @@ * @see https://github.com/VolvoxLLC/volvox-bot/issues/177 */ +import { MS_PER_MINUTE } from '../constants/index.js'; import { debug, warn } from '../logger.js'; import { getRedis, recordError, recordHit, recordMiss } from '../redis.js'; @@ -28,9 +29,11 @@ export const TTL = { CHANNEL_DETAIL: Number(process.env.REDIS_TTL_CHANNEL_DETAIL) || 600, // 10 min }; +/** Maximum number of entries in the in-memory LRU cache */ +const MAX_MEMORY_CACHE_SIZE = 1000; + /** @type {Map} In-memory LRU fallback */ const memoryCache = new Map(); -const MAX_MEMORY_CACHE_SIZE = 1000; /** Interval reference for memory cache cleanup */ let cleanupInterval = null; @@ -48,7 +51,7 @@ function ensureCleanup() { memoryCache.delete(key); } } - }, 60_000); + }, MS_PER_MINUTE); cleanupInterval.unref(); } diff --git a/src/utils/duration.js b/src/utils/duration.js index 0ea71af4..f93d93a9 100644 --- a/src/utils/duration.js +++ b/src/utils/duration.js @@ -5,18 +5,27 @@ * into milliseconds and format milliseconds back into readable strings. */ +import { + MS_PER_DAY, + MS_PER_HOUR, + MS_PER_MINUTE, + MS_PER_SECOND, + MS_PER_WEEK, + MS_PER_YEAR, +} from '../constants/index.js'; + const UNITS = { - s: 1000, - m: 60 * 1000, - h: 60 * 60 * 1000, - d: 24 * 60 * 60 * 1000, - w: 7 * 24 * 60 * 60 * 1000, + s: MS_PER_SECOND, + m: MS_PER_MINUTE, + h: MS_PER_HOUR, + d: MS_PER_DAY, + w: MS_PER_WEEK, }; const DURATION_RE = /^\s*(\d+)\s*([smhdw])\s*$/i; /** Maximum allowed duration: 1 year in milliseconds. */ -const MAX_DURATION_MS = 365 * 24 * 60 * 60 * 1000; +const MAX_DURATION_MS = MS_PER_YEAR; /** * Parse a duration string into milliseconds. diff --git a/src/utils/guildSpend.js b/src/utils/guildSpend.js index 4b735fcf..07dbd316 100644 --- a/src/utils/guildSpend.js +++ b/src/utils/guildSpend.js @@ -6,6 +6,7 @@ * gate evaluations when a guild exceeds its daily AI budget. */ +import { MS_PER_DAY } from '../constants/index.js'; import { getPool } from '../db.js'; import { warn } from '../logger.js'; @@ -19,7 +20,7 @@ import { warn } from '../logger.js'; * @param {number} [windowMs=86400000] - Rolling window in milliseconds (default: 24 h). * @returns {Promise} Total spend in USD for the window period. */ -export async function getGuildSpend(guildId, windowMs = 24 * 60 * 60 * 1000) { +export async function getGuildSpend(guildId, windowMs = MS_PER_DAY) { if (!guildId) return 0; let pool; @@ -55,7 +56,7 @@ export async function getGuildSpend(guildId, windowMs = 24 * 60 * 60 * 1000) { * @param {number} [windowMs=86400000] - Rolling window in milliseconds (default: 24 h). * @returns {Promise<{status: 'ok'|'warning'|'exceeded', spend: number, budget: number, pct: number}>} */ -export async function checkGuildBudget(guildId, dailyBudgetUsd, windowMs = 24 * 60 * 60 * 1000) { +export async function checkGuildBudget(guildId, dailyBudgetUsd, windowMs = MS_PER_DAY) { const spend = await getGuildSpend(guildId, windowMs); const pct = dailyBudgetUsd > 0 ? spend / dailyBudgetUsd : 0; diff --git a/src/utils/permissions.js b/src/utils/permissions.js index f1fe4afd..0873ae16 100644 --- a/src/utils/permissions.js +++ b/src/utils/permissions.js @@ -117,19 +117,35 @@ export function hasPermission(member, commandName, config) { } /** - * Check if a member is a guild admin (has ADMINISTRATOR permission or bot admin role). + * Check if a member is a guild admin (has ADMINISTRATOR permission, bot admin role, or guild-scoped admin roles). * - * Currently delegates to {@link isAdmin}. This is an intentional alias to establish - * a separate semantic entry-point for per-guild admin checks. When per-guild config - * lands (Issue #71), this function will diverge to check guild-scoped admin roles - * instead of the global bot admin role. + * Checks guild-scoped admin roles from per-guild config (permissions.adminRoles array), + * then falls back to global admin checks via {@link isAdmin}. * * @param {GuildMember} member - Discord guild member - * @param {Object} config - Bot configuration + * @param {Object} config - Bot configuration (should be guild-specific via getConfig(guildId)) * @returns {boolean} True if member is a guild admin */ export function isGuildAdmin(member, config) { - // TODO(#71): check guild-scoped admin roles once per-guild config is implemented + if (!member) return false; + + // Bot owner always bypasses permission checks + if (isBotOwner(member, config)) return true; + + // Check if member has Discord Administrator permission + if (member.permissions?.has(PermissionFlagsBits.Administrator)) { + return true; + } + + // Check guild-scoped admin roles (per-guild config) + const adminRoles = config?.permissions?.adminRoles; + if (Array.isArray(adminRoles) && adminRoles.length > 0) { + if (adminRoles.some((roleId) => member.roles?.cache?.has(roleId))) { + return true; + } + } + + // Fall back to global admin checks (single adminRoleId, etc.) return isAdmin(member, config); } diff --git a/tasks/task-001.md b/tasks/task-001.md new file mode 100644 index 00000000..4571b4af --- /dev/null +++ b/tasks/task-001.md @@ -0,0 +1,65 @@ +# Task 001: Refactor guilds.js God Route + +## Parent +- **Master Task:** Code improvements from CODE_IMPROVEMENTS.md +- **Branch:** refactor/guilds-routes + +## Context +The file `src/api/routes/guilds.js` is 1,622 lines and handles too many concerns: +- Analytics endpoints (charts, stats, activity) +- Member management (list, search, bulk actions) +- Guild configuration +- Moderation actions + +This violates single responsibility principle and makes the file hard to maintain. + +## Files to Work On +- `src/api/routes/guilds.js` - Source file to split +- Create: `src/api/routes/analytics.js` - Analytics endpoints +- Create: `src/api/routes/members.js` - Already exists but may need cleanup +- Create: `src/api/routes/guildConfig.js` - Config-specific routes +- Update: `src/api/index.js` - Register new routes +- Update tests as needed + +## Requirements + +### Phase 1: Extract Analytics Routes +Move these endpoints from guilds.js to analytics.js: +- `GET /:id/analytics/activity` - Message activity data +- `GET /:id/analytics/commands` - Command usage stats +- `GET /:id/analytics/voice` - Voice channel stats +- `GET /:id/analytics/overview` - Summary statistics +- `GET /:id/analytics/export` - CSV/JSON export + +### Phase 2: Extract Config Routes +Move these endpoints to guildConfig.js: +- `GET /:id/config` - Get guild config +- `PATCH /:id/config` - Update config +- `POST /:id/config/reset` - Reset to defaults + +### Phase 3: Cleanup guilds.js +After extraction, guilds.js should only handle: +- Basic guild info endpoints +- Guild validation middleware exports + +## Constraints +- Do NOT change API behavior - this is pure refactoring +- Keep all existing tests passing +- Export shared functions (like `parsePagination`, `parseAnalyticsRange`) from a utils file +- Update imports in index.js + +## Acceptance Criteria +- [ ] analytics.js created with all analytics endpoints +- [ ] guildConfig.js created with config endpoints +- [ ] guilds.js reduced to <500 lines +- [ ] All existing tests pass +- [ ] No API behavior changes +- [ ] Proper JSDoc on extracted functions +- [ ] Code passes lint check + +## Results +_[To be filled by subagent]_ + +**Status:** [In Progress] +**Commits:** +**Issues:** diff --git a/tasks/task-002.md b/tasks/task-002.md new file mode 100644 index 00000000..57278e19 --- /dev/null +++ b/tasks/task-002.md @@ -0,0 +1,70 @@ +# Task 002: Add Missing Tests + +## Parent +- **Master Task:** Code improvements from CODE_IMPROVEMENTS.md +- **Branch:** refactor/missing-tests + +## Context +Several modules have no test coverage: +- `src/modules/pollHandler.js` - Poll voting logic +- `src/modules/reputationDefaults.js` - Default reputation config +- `src/modules/reviewHandler.js` - Review claim handling +- `src/utils/cronParser.js` - Cron expression parsing +- `src/utils/flattenToLeafPaths.js` - Object flattening utility + +## Files to Create +- `tests/modules/pollHandler.test.js` +- `tests/modules/reputationDefaults.test.js` +- `tests/modules/reviewHandler.test.js` +- `tests/utils/cronParser.test.js` +- `tests/utils/flattenToLeafPaths.test.js` + +## Requirements + +### pollHandler.test.js +Test the poll voting logic: +- Handle poll vote reactions +- Validate vote eligibility +- Update poll results +- Close polls and announce winners + +### reputationDefaults.test.js +Test default reputation configuration: +- Default XP values +- Level thresholds +- Role rewards structure + +### reviewHandler.test.js +Test review claim handling: +- Claim review items +- Unclaim/release reviews +- Complete reviews with feedback +- Prevent double-claiming + +### cronParser.test.js +Test cron expression parsing: +- Parse standard cron formats +- Handle special characters (*, /, -) +- Calculate next run times +- Error on invalid expressions + +### flattenToLeafPaths.test.js +Test object flattening: +- Flatten nested objects to dot paths +- Handle arrays +- Preserve primitive values +- Edge cases (null, empty objects) + +## Acceptance Criteria +- [ ] All 5 test files created +- [ ] Tests cover main functionality +- [ ] Tests pass (`pnpm test`) +- [ ] Coverage meets project standards (>80%) +- [ ] No lint errors + +## Results +_[To be filled by subagent]_ + +**Status:** [In Progress] +**Commits:** +**Issues:** diff --git a/tests/utils/permissions.test.js b/tests/utils/permissions.test.js index 683d0b7a..20750c29 100644 --- a/tests/utils/permissions.test.js +++ b/tests/utils/permissions.test.js @@ -345,6 +345,101 @@ describe('isGuildAdmin', () => { }; expect(isGuildAdmin(member, null)).toBe(false); }); + + it('should return true for members with guild-scoped admin role (adminRoles array)', () => { + const member = { + permissions: { has: vi.fn().mockReturnValue(false) }, + roles: { + cache: { + has: vi.fn().mockImplementation((roleId) => roleId === 'guild-admin-role-1'), + }, + }, + }; + const config = { + permissions: { + adminRoles: ['guild-admin-role-1', 'guild-admin-role-2'], + }, + }; + expect(isGuildAdmin(member, config)).toBe(true); + }); + + it('should return true for members with any role in adminRoles array', () => { + const member = { + permissions: { has: vi.fn().mockReturnValue(false) }, + roles: { + cache: { + has: vi.fn().mockImplementation((roleId) => roleId === 'guild-admin-role-2'), + }, + }, + }; + const config = { + permissions: { + adminRoles: ['guild-admin-role-1', 'guild-admin-role-2'], + }, + }; + expect(isGuildAdmin(member, config)).toBe(true); + }); + + it('should return false for members without guild-scoped admin roles', () => { + const member = { + permissions: { has: vi.fn().mockReturnValue(false) }, + roles: { cache: { has: vi.fn().mockReturnValue(false) } }, + }; + const config = { + permissions: { + adminRoles: ['guild-admin-role-1', 'guild-admin-role-2'], + }, + }; + expect(isGuildAdmin(member, config)).toBe(false); + }); + + it('should fall back to adminRoleId when adminRoles is empty', () => { + const member = { + permissions: { has: vi.fn().mockReturnValue(false) }, + roles: { cache: { has: vi.fn().mockReturnValue(true) } }, + }; + const config = { + permissions: { + adminRoles: [], + adminRoleId: 'global-admin-role', + }, + }; + expect(isGuildAdmin(member, config)).toBe(true); + expect(member.roles.cache.has).toHaveBeenCalledWith('global-admin-role'); + }); + + it('should handle empty adminRoles array gracefully', () => { + const member = { + permissions: { has: vi.fn().mockReturnValue(false) }, + roles: { cache: { has: vi.fn().mockReturnValue(false) } }, + }; + const config = { + permissions: { + adminRoles: [], + }, + }; + expect(isGuildAdmin(member, config)).toBe(false); + }); + + it('should prioritize adminRoles over adminRoleId when both are set', () => { + const member = { + permissions: { has: vi.fn().mockReturnValue(false) }, + roles: { + cache: { + has: vi.fn().mockImplementation((roleId) => roleId === 'guild-admin-role-1'), + }, + }, + }; + const config = { + permissions: { + adminRoles: ['guild-admin-role-1'], + adminRoleId: 'global-admin-role', + }, + }; + expect(isGuildAdmin(member, config)).toBe(true); + // Should match on adminRoles without checking adminRoleId + expect(member.roles.cache.has).toHaveBeenCalledWith('guild-admin-role-1'); + }); }); describe('isModerator', () => {