feat: REST API layer for web dashboard#70
Conversation
Add Express HTTP server running alongside the Discord WebSocket client, providing a REST API for the upcoming web dashboard. Endpoints: - GET /api/v1/health — uptime, memory, discord status - GET /api/v1/guilds/:id — guild info via Discord client - GET /api/v1/guilds/:id/config — read config - PATCH /api/v1/guilds/:id/config — update config value - GET /api/v1/guilds/:id/stats — AI stats, message counts from DB - GET /api/v1/guilds/:id/members — paginated member list with roles - GET /api/v1/guilds/:id/moderation — paginated mod cases from DB - POST /api/v1/guilds/:id/actions — bot actions (sendMessage) Middleware: - x-api-secret header authentication (BOT_API_SECRET) - Per-IP rate limiting (in-memory) - CORS restricted to DASHBOARD_URL origin - JSON body parsing, global error handler Structure: - src/api/server.js — Express app, middleware, start/stop - src/api/index.js — router mount - src/api/middleware/auth.js — auth middleware - src/api/middleware/rateLimit.js — rate limiter - src/api/routes/health.js — health endpoint - src/api/routes/guilds.js — guild endpoints Integration: - Starts in src/index.js after Discord login - Graceful shutdown before DB close Tests: 5 test files, all endpoints tested with mocked Discord client and DB pool. 80%+ coverage on all metrics. Closes #29
|
Claude Code is working… I'll analyze this and get back to you. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an Express REST API (health and guild endpoints) with timing-safe x-api-secret auth, in-memory per-IP rate limiting, and lifecycle functions (createApp/startServer/stopServer). Threads optional guildId into AI history and conversations, updates example envs, DB schema, and adds comprehensive tests. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
1 critical, 5 warnings, 2 nitpicks found across the REST API implementation.
🔴 Critical (must fix)
- Mention sanitization bypass (
guilds.js:222):channel.send(content)sends raw user input to Discord without usingsafeSend(). An API caller could trigger@everyone/@herepings, bypassing the defense-in-depth sanitization used everywhere else in the codebase.
🟡 Warnings (should fix)
- Timing-safe secret comparison (
auth.js:24): String comparison vulnerable to timing attacks. Usecrypto.timingSafeEqual(). - Config endpoint exposes full config (
guilds.js:69):GET /:id/configreturns the entire bot config, potentially including sensitive data (DB credentials, API keys, internal URLs). - Config PATCH is not guild-scoped (
guilds.js:77-96): The guild ID parameter is cosmetic —setConfigValue()modifies global config. No allowlist restricts which paths can be modified via API. - Conversations query not guild-scoped (
guilds.js:110): Stats endpoint counts all conversations globally, not filtered to the requested guild. - Members pagination is broken (
guilds.js:133-137):guild.members.fetch({ limit })doesn't support offset — page 2+ returns the same results as page 1.
🔵 Nitpicks
- User input reflected in error response (
guilds.js:230) - CORS
Access-Control-Allow-MethodsincludesDELETEbut no DELETE routes exist
See inline comments for details and suggested fixes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.env.examplepackage.jsonsrc/api/index.jssrc/api/middleware/auth.jssrc/api/middleware/rateLimit.jssrc/api/routes/guilds.jssrc/api/routes/health.jssrc/api/server.jssrc/index.jstests/api/middleware/auth.test.jstests/api/middleware/rateLimit.test.jstests/api/routes/guilds.test.jstests/api/routes/health.test.jstests/api/server.test.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules withimport/exportsyntax, never userequire()
Always usenode:protocol prefix for Node.js builtin imports (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals, enforced by Biome
Use 2-space indentation throughout the codebase, enforced by Biome
Files:
src/api/middleware/auth.jstests/api/server.test.jstests/api/middleware/auth.test.jssrc/api/routes/guilds.jssrc/api/server.jssrc/api/index.jssrc/api/routes/health.jstests/api/routes/health.test.jssrc/api/middleware/rateLimit.jstests/api/routes/guilds.test.jstests/api/middleware/rateLimit.test.jssrc/index.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Never useconsole.log(),console.warn(),console.error(), or any otherconsole.*method in src/ files. Always use Winston logger instead:import { info, warn, error } from '../logger.js'and log with structured metadata
Use custom error classes fromsrc/utils/errors.jsfor error handling. Always log errors with context before re-throwing
UsesplitMessage()utility to handle Discord's 2000-character message limit for outgoing messages
UsesafeSend()wrappers fromsrc/utils/safeSend.jsfor all outgoing messages, and usesanitizeMentions()fromsrc/utils/sanitizeMentions.jsto strip@everyone/@here via zero-width space insertion
Any new code must include tests. Runpnpm testbefore every commit. Maintain minimum 80% code coverage on statements, branches, functions, and lines using@vitest/coverage-v8. PRs that drop coverage below 80% will fail CI
Write JSDoc comments for documentation instead of TypeScript, as the project uses plain JavaScript without TypeScript
Files:
src/api/middleware/auth.jssrc/api/routes/guilds.jssrc/api/server.jssrc/api/index.jssrc/api/routes/health.jssrc/api/middleware/rateLimit.jssrc/index.js
src/index.js
📄 CodeRabbit inference engine (AGENTS.md)
src/index.js: Enforce Discord intents: the bot requires MessageContent, GuildMembers, and GuildVoiceStates intents to be enabled insrc/index.jsclient setup
Tempban scheduler runs on a 60-second interval. Started insrc/index.jsstartup and stopped in graceful shutdown. Catches up on missed unbans after restart
The PostgreSQL logging transport is a long-lived Winston transport that requires reactiveonConfigChangewiring insrc/index.jsstartup to add/remove/recreate the transport whenlogging.database.*settings change at runtime
Files:
src/index.js
🧠 Learnings (2)
📚 Learning: 2025-10-10T15:05:26.145Z
Learnt from: CR
Repo: BillChirico/LUA-Obfuscator PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-10T15:05:26.145Z
Learning: Applies to package.json : Only add new packages when absolutely necessary or explicitly requested
Applied to files:
package.json
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/**/*.js : Any new code must include tests. Run `pnpm test` before every commit. Maintain minimum 80% code coverage on statements, branches, functions, and lines using vitest/coverage-v8. PRs that drop coverage below 80% will fail CI
Applied to files:
package.jsontests/api/server.test.js
🧬 Code graph analysis (11)
src/api/middleware/auth.js (1)
src/logger.js (1)
warn(224-226)
tests/api/server.test.js (1)
src/api/server.js (6)
stopServer(89-107)app(22-22)app(68-68)createApp(21-58)server(12-12)startServer(67-82)
tests/api/middleware/auth.test.js (1)
src/api/middleware/auth.js (1)
requireAuth(14-31)
src/api/routes/guilds.js (3)
src/index.js (2)
client(79-88)config(57-57)src/modules/config.js (3)
getConfig(133-135)setConfigValue(222-311)err(33-33)src/logger.js (2)
info(217-219)error(231-233)
src/api/index.js (3)
src/api/routes/guilds.js (1)
router(10-10)src/api/routes/health.js (1)
router(8-8)src/api/middleware/auth.js (1)
requireAuth(14-31)
src/api/routes/health.js (3)
src/api/index.js (1)
router(11-11)src/api/routes/guilds.js (6)
router(10-10)req(32-32)req(78-78)req(102-102)req(164-164)req(200-200)src/index.js (1)
client(79-88)
tests/api/routes/health.test.js (2)
src/index.js (1)
client(79-88)src/api/server.js (3)
createApp(21-58)app(22-22)app(68-68)
src/api/middleware/rateLimit.js (1)
tests/api/middleware/rateLimit.test.js (3)
req(6-6)res(7-7)next(8-8)
tests/api/routes/guilds.test.js (2)
src/api/server.js (3)
app(22-22)app(68-68)createApp(21-58)src/modules/config.js (2)
getConfig(133-135)setConfigValue(222-311)
tests/api/middleware/rateLimit.test.js (1)
src/api/middleware/rateLimit.js (1)
rateLimit(15-52)
src/index.js (1)
src/api/server.js (2)
stopServer(89-107)startServer(67-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (6)
.env.example (1)
47-59: LGTM!Environment variable entries are well-documented with sensible defaults and align with the new API server configuration needs.
src/api/index.js (1)
1-19: LGTM!Clean route aggregation with appropriate separation of public (
/health) and authenticated (/guilds) endpoints.tests/api/routes/health.test.js (1)
1-50: LGTM!Good test structure — the mock client accurately mirrors the shape accessed by the health route, and the tests validate both response structure and the public (no-auth) access requirement.
package.json (1)
22-22: Express 5.2.1 is stable and the code is compatible.Express 5.0.0 has been stable since September 2024, became the npm default in March 2025, and is now well-established (February 2026). The route handlers and middleware use no deprecated Express methods and are fully compatible with Express 5's semantics—
req.query,req.params,req.body, and middleware patterns all work correctly.tests/api/middleware/auth.test.js (1)
1-70: LGTM!Good coverage of all auth middleware branches — unconfigured secret, missing header, mismatched header, and valid header. Mock setup and teardown are clean.
tests/api/routes/guilds.test.js (1)
1-387: LGTM!Thorough test coverage across all guild endpoints — happy paths, validation errors, DB unavailability, and Discord API failures. Good verification of parameterized queries for SQL injection prevention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/middleware/auth.js`:
- Around line 24-27: Replace the plain equality check with a timing-safe
comparison using node:crypto: import crypto and convert secret and expected to
Buffers (e.g. secretBuf = Buffer.from(secret || '') and expectedBuf =
Buffer.from(expected)); if their lengths differ, call crypto.timingSafeEqual
against expectedBuf and a dummy Buffer.alloc(expectedBuf.length) to keep timing
consistent, then return 401; if lengths match, use
crypto.timingSafeEqual(secretBuf, expectedBuf) and if it returns false, log the
warn('Unauthorized API request', { ip: req.ip, path: req.path }) and return
res.status(401).json({ error: 'Unauthorized' }). Ensure you update the
conditional in the middleware (the block containing secret and expected) and
keep the existing log and response behavior.
In `@src/api/middleware/rateLimit.js`:
- Around line 15-51: The rateLimit factory currently creates a cleanup interval
that is never cleared; update rateLimit to expose a way to stop that interval by
returning the middleware function augmented with a cleanup method (e.g.,
middleware.close or middleware.destroy) that calls clearInterval(cleanup) (keep
cleanup.unref()); specifically, in rateLimit() keep the const cleanup =
setInterval(...) and then before returning create const middleware =
(req,res,next)=>{...}; add middleware.destroy = () => clearInterval(cleanup);
and return middleware instead of the raw function so callers (tests or app
shutdown) can call middleware.destroy() to avoid timer leaks.
In `@src/api/routes/guilds.js`:
- Around line 133-137: The members endpoint currently calls
req.guild.members.fetch({ limit }) and ignores offset-based page semantics from
parsePagination; update router.get('/:id/members') to support cursor-based
pagination: accept an 'after' (snowflake) query param instead of (or in addition
to) page, pass { limit, after } into req.guild.members.fetch, return the fetched
members plus the next cursor (the last member's id) so clients can page forward;
also adjust parsePagination usage and response shape to include the next_after
cursor (or remove/ignore page entirely and document the change) to avoid
misleading page-based pagination behavior.
- Around line 229-231: The response currently reflects unsanitized user input
(action) back to the client in res.status(400).json({ error: `Unknown action:
${action}` });; update the error to avoid echoing raw input by returning a
generic message such as { error: "Unknown action" } or, if you must include the
value, include a sanitized/truncated form (e.g., escape and limit length) before
adding it to the JSON. Locate the handler that sets this response and replace
the template interpolation of action with the generic text or sanitized
variable.
- Around line 221-224: The code sends raw user-supplied content via
channel.send, which can ping everyone and exceed Discord limits; replace the
direct channel.send with the safeSend wrapper and sanitize the content first by
calling sanitizeMentions(req.body.content) (or sanitizeMentions(content)) to
strip `@everyone/`@here, then call safeSend(channel, sanitizedContent) instead of
channel.send; update the response to use the returned message from safeSend
(message.id) and ensure sanitizeMentions and safeSend are imported/required in
this module; remove any direct channel.send usage in this try block.
- Around line 109-114: The conversations count query is currently unscoped and
returns a global total; update the dbPool.query that produces conversationResult
to restrict conversations to the requested guild by joining conversations to the
channels table on conversations.channel_id = channels.id (or the correct PK) and
adding WHERE channels.guild_id = $1 (use req.params.id as the parameter), e.g.
replace the SELECT COUNT(*)... FROM conversations with a SELECT COUNT(*)::int AS
count FROM conversations JOIN channels ON conversations.channel_id = channels.id
WHERE channels.guild_id = $1; keep the same Promise.all structure and parameter
ordering so conversationResult and caseResult remain unchanged.
In `@src/api/routes/health.js`:
- Around line 14-27: The health route handler (router.get) currently returns
full process.memoryUsage() and Discord internals (client.ws.status,
client.ws.ping, client.guilds.cache.size) to unauthenticated callers; remove or
sanitize detailed memory metrics and/or gate the detailed response behind
authentication. Update the handler so the public response only includes minimal
info (e.g., status: 'ok' and uptime) and move detailed fields (memory object,
client.ws.*, guild counts) behind an auth check or an isAdmin middleware; modify
the router.get callback to check req.user/auth and conditionally attach the
detailed diagnostics instead of always returning process.memoryUsage().
In `@src/api/server.js`:
- Around line 67-82: The startServer function overwrites the module-scoped
server variable and leaks an existing HTTP server; update startServer to detect
an existing server (the module-level server variable), gracefully close it (call
server.close and await its callback or wrap in a Promise), handle/propagate any
close errors, clear the old reference, then create and assign the new server
from createApp(...). Ensure the server 'error' and 'listening' handlers remain
attached to the new instance and that server is set to null after a successful
close or on failure to avoid orphaned sockets.
- Around line 33-43: The middleware registered with app.use (the anonymous
function that references dashboardUrl) currently returns res.status(204) for all
OPTIONS requests even when dashboardUrl is falsy; change the logic so the
preflight short-circuit only runs when dashboardUrl is set: set the CORS headers
when dashboardUrl is truthy and only return 204 for OPTIONS if dashboardUrl is
truthy (otherwise call next()), updating the anonymous middleware in
src/api/server.js that uses dashboardUrl to guard the OPTIONS response.
In `@src/index.js`:
- Around line 456-458: The startup sequence currently awaits startServer(client,
dbPool) without handling rejection, which can leave client logged in, schedulers
running, and dbPool open if the server fails; wrap the startServer call in a
try/catch inside startup() and on error either (a) log a warning and continue
(so the bot remains functional without the REST API) or (b) call
gracefulShutdown() to clean up client and dbPool then rethrow/exit; reference
the startServer(...) call, startup(), gracefulShutdown(), client, and dbPool
when making the change.
In `@tests/api/middleware/rateLimit.test.js`:
- Around line 67-87: The test uses vi.useFakeTimers() but calls
vi.useRealTimers() only at the end of the test, which can leak fake timers if an
assertion fails; change the test to always restore timers by either adding a
global afterEach(() => vi.useRealTimers()) for this test suite or wrap the
fake-timer section in a try/finally so vi.useRealTimers() is guaranteed to run;
refer to the rateLimit.test.js test case (the it block creating middleware via
rateLimit({ windowMs: 1000, max: 1 })) and ensure vi.useFakeTimers() is paired
with a guaranteed cleanup (vi.useRealTimers()) to avoid affecting other tests.
In `@tests/api/server.test.js`:
- Around line 42-59: Remove the redundant vi.unstubAllEnvs() calls from the
individual tests (e.g., inside the "should parse JSON request bodies" test and
the other tests that duplicate cleanup); the global afterEach cleanup already
calls vi.unstubAllEnvs(), so simply delete the per-test vi.unstubAllEnvs()
invocations and leave the tests to rely on the existing afterEach block to
restore env stubs.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace channel.send() with safeSend() in the actions endpoint to ensure mention sanitization and safe allowedMentions. Add content length validation to reject messages exceeding Discord's 2000-char limit. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add SAFE_CONFIG_KEYS allowlist to only expose safe config sections (ai, welcome, spam, moderation, logging) via GET. Restrict PATCH to only allow modifying keys under safe prefixes, returning 403 otherwise. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Move the OPTIONS 204 response inside the dashboardUrl check so that when CORS is unconfigured, OPTIONS requests fall through to normal routing instead of returning an empty 204. Co-Authored-By: Claude Opus 4.6 <[email protected]>
If startServer is called while a server is already running, close the existing server first to prevent orphaned listeners leaking resources. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Discord API does not support offset-based pagination for members. Switch from guild.members.fetch() to guild.members.list() which uses cursor-based pagination with an "after" parameter (user ID). The endpoint now accepts ?limit=25&after=<userId> instead of ?page=1&limit=25 and returns nextAfter cursor for the next page. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Remove process.memoryUsage() from the default health response to avoid exposing server internals without authentication. Memory info is now only included when a valid x-api-secret header is provided. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Wrap startServer in try/catch so the Discord bot continues running even if the HTTP API server fails to bind its port. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The conversations table has no guild_id column, so we scope the stats count to the requesting guild by filtering on channel_id using the guild's channel cache (channel_id = ANY). Resolves PR #70 review threads: - PRRT_kwDORICdSM5u9e-S (conversations query not guild-scoped) - PRRT_kwDORICdSM5u9fwJ (same issue, coderabbitai) - PRRT_kwDORICdSM5u9fG8 (same issue, claude)
Move vi.useRealTimers() to afterEach so timers are always restored even if a test fails before the inline cleanup runs. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Remove redundant vi.unstubAllEnvs() calls from individual tests and add it to the shared afterEach block for consistent cleanup. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
2 warnings found on the current code. CI passes and previous review iterations have addressed the major issues well.
🟡 Warnings
-
Unguarded
CREATE INDEXinsrc/db.js:122-127: TheCREATE INDEX IF NOT EXISTS idx_conversations_guild_idis outside the try/catch that wraps theALTER TABLE. If the ALTER TABLE fails (PG < 9.6) on an existing database where the column doesn't exist, this throws an unhandled error that crashesinitDb()and prevents bot startup. Wrap it in its own try/catch. -
Shutdown timeout doesn't resolve promise on Node < 18.2 (
src/api/server.js:134-139): WhencloseAllConnectionsis unavailable, the timeout fires, logs a warning, but thestopServer()promise never resolves — hanging graceful shutdown indefinitely. Add a fallbackresolve()in the else branch.
See inline comments for suggested fixes.
There was a problem hiding this comment.
2 warnings, 2 nitpicks found. CI passes. The implementation has been iterated extensively (70 commits) and the vast majority of issues from prior review rounds have been well-addressed.
🟡 Warnings
CREATE INDEXonguild_idwill crashinitDbifALTER TABLEfailed (src/db.js:122-127): The index creation is outside the try/catch that guards theALTER TABLE ADD COLUMN IF NOT EXISTS. On PG < 9.6 where the ALTER fails, this will throw an unhandled error and prevent bot startup. Wrap in its own try/catch.- Health endpoint doesn't guard against
client.wsbeing unavailable (src/api/routes/health.js:18): If the health check is hit before Discord's WebSocket is fully initialized, accessingclient.ws.statuscould throw.
🔵 Nitpicks
- Error handler logs full stack trace for 4xx client errors (
src/api/server.js:63): Adds noise to logs for expected errors like malformed JSON. - Missing explicit
sanitizeMentions()call beforesafeSend(src/api/routes/guilds.js:307): Not a security issue (safeSend sanitizes internally), but deviates from the project convention stated in AGENTS.md.
See inline comments for details.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
- Add explicit sanitizeMentions() call before safeSend in guilds actions endpoint - Wrap CREATE INDEX in try/catch for PG < 9.6 compatibility - Add client?.ws defensive guard in health endpoint - Rename _req to req in config GET handler - Add resolve() to shutdown timeout so promise completes - Only log stack traces for 5xx errors, not 4xx - Validate config path has no empty segments
There was a problem hiding this comment.
LGTM — this PR has been through extensive review iteration (71 commits, 10+ review rounds) and all critical and warning-level issues have been addressed. The 2 remaining nitpicks (double sanitization, channel type enum documentation) are non-blocking.
Key items verified as resolved: timing-safe auth with shared isValidSecret helper, safeSend for mention sanitization + message splitting, config key allowlist (SAFE_CONFIG_KEYS/READABLE_CONFIG_KEYS), cursor-based member pagination, input reflection prevention, CORS OPTIONS guarding, startServer double-start protection, non-fatal API startup, conversations guild scoping, health endpoint auth gating, NaN port handling with range validation, rateLimit destroy() lifecycle, shutdown timeout with closeAllConnections(), error handler respecting err.status for 4xx codes, req.body guard for Express 5, string type check for content, explicit column list in moderation query, documentation updates in AGENTS.md and README.md.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.mdsrc/api/routes/guilds.jssrc/api/routes/health.jssrc/api/server.jssrc/db.jstests/api/server.test.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules withimport/exportsyntax, never userequire()
Always usenode:protocol prefix for Node.js builtin imports (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals, enforced by Biome
Use 2-space indentation throughout the codebase, enforced by Biome
Files:
src/api/server.jssrc/api/routes/health.jstests/api/server.test.jssrc/api/routes/guilds.jssrc/db.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Never useconsole.log(),console.warn(),console.error(), or any otherconsole.*method in src/ files. Always use Winston logger instead:import { info, warn, error } from '../logger.js'and log with structured metadata
Use custom error classes fromsrc/utils/errors.jsfor error handling. Always log errors with context before re-throwing
UsesplitMessage()utility to handle Discord's 2000-character message limit for outgoing messages
UsesafeSend()wrappers fromsrc/utils/safeSend.jsfor all outgoing messages, and usesanitizeMentions()fromsrc/utils/sanitizeMentions.jsto strip@everyone/@here via zero-width space insertion
Any new code must include tests. Runpnpm testbefore every commit. Maintain minimum 80% code coverage on statements, branches, functions, and lines using@vitest/coverage-v8. PRs that drop coverage below 80% will fail CI
Write JSDoc comments for documentation instead of TypeScript, as the project uses plain JavaScript without TypeScript
Files:
src/api/server.jssrc/api/routes/health.jssrc/api/routes/guilds.jssrc/db.js
🧠 Learnings (11)
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Keep documentation files up to date after every code change: update README.md for setup/config changes, update AGENTS.md for architecture/key files changes, update CONTRIBUTING.md for workflow changes, update .env.example for environment variables, update config.json for new config sections
Applied to files:
src/api/server.js
📚 Learning: 2026-02-11T17:18:14.598Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-11T17:18:14.598Z
Learning: See AGENTS.md for full project context, architecture, and coding guidelines
Applied to files:
src/api/server.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/**/*.js : Use custom error classes from `src/utils/errors.js` for error handling. Always log errors with context before re-throwing
Applied to files:
src/api/server.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/**/*.js : Any new code must include tests. Run `pnpm test` before every commit. Maintain minimum 80% code coverage on statements, branches, functions, and lines using vitest/coverage-v8. PRs that drop coverage below 80% will fail CI
Applied to files:
tests/api/server.test.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/**/*.js : Use `safeSend()` wrappers from `src/utils/safeSend.js` for all outgoing messages, and use `sanitizeMentions()` from `src/utils/sanitizeMentions.js` to strip everyone/here via zero-width space insertion
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/**/*.js : Use `splitMessage()` utility to handle Discord's 2000-character message limit for outgoing messages
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/modules/moderation.js : Case numbering must be per-guild sequential and assigned atomically using `COALESCE(MAX(case_number), 0) + 1` in a single INSERT statement inside `createCase()`
Applied to files:
src/api/routes/guilds.jssrc/db.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/index.js : Enforce Discord intents: the bot requires MessageContent, GuildMembers, and GuildVoiceStates intents to be enabled in `src/index.js` client setup
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/index.js : The PostgreSQL logging transport is a long-lived Winston transport that requires reactive `onConfigChange` wiring in `src/index.js` startup to add/remove/recreate the transport when `logging.database.*` settings change at runtime
Applied to files:
src/api/routes/guilds.jssrc/db.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/commands/**/*.js : Discord timeouts have a maximum duration of 28 days; Discord slowmode has a maximum duration of 6 hours (21600 seconds). Enforce these caps in command logic
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/**/*.js : Never use `console.log()`, `console.warn()`, `console.error()`, or any other `console.*` method in src/ files. Always use Winston logger instead: `import { info, warn, error } from '../logger.js'` and log with structured metadata
Applied to files:
src/db.js
🧬 Code graph analysis (4)
src/api/server.js (5)
src/index.js (2)
client(79-88)dbPool(308-308)src/api/routes/guilds.js (5)
req(57-57)req(126-126)req(166-166)req(241-241)req(281-281)src/api/routes/health.js (1)
req(18-18)src/api/middleware/rateLimit.js (1)
rateLimit(15-56)src/logger.js (3)
error(231-233)warn(224-226)info(217-219)
src/api/routes/health.js (3)
src/api/index.js (1)
router(11-11)src/index.js (1)
client(79-88)src/api/middleware/auth.js (1)
isValidSecret(15-24)
src/api/routes/guilds.js (4)
src/modules/config.js (3)
getConfig(133-135)setConfigValue(222-311)err(33-33)src/logger.js (2)
info(217-219)error(231-233)src/utils/sanitizeMentions.js (1)
sanitizeMentions(37-43)src/utils/safeSend.js (1)
safeSend(116-123)
src/db.js (1)
src/logger.js (1)
warn(224-226)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (10)
src/db.js (2)
7-7: LGTM!Import of
warnfrom the local logger is consistent with existinginfoandlogErrorimports and follows the project's logging guidelines.
104-104: LGTM!Adding
guild_id TEXT(nullable) to theCREATE TABLEis appropriate — new rows can populate it while existing databases get backfilled separately.README.md (1)
374-380: LGTM — documentation updates accurately reflect the new REST API surface.The README correctly documents the bot's REST API server, its default port, and service details.
Also applies to: 447-447
src/api/routes/health.js (1)
1-46: LGTM — well-structured health endpoint with appropriate auth gating.Previous review concerns (duplicated timing-safe logic, unauthenticated memory exposure) have been properly addressed. The defensive guard for pre-login state and conditional detail exposure are solid.
src/api/server.js (3)
36-45: CORS middleware correctly guards preflight behindDASHBOARD_URL.Past review concern about OPTIONS responding 204 without CORS headers has been addressed. The early return on falsy
dashboardUrlis clean.
87-117: Server lifecycle is well-structured with proper double-start guard and port validation.Previous review concerns (double-start leak, NaN port, port range, startup error listener) have all been addressed. The
once('error', ...)pattern for the startup listener is correct.
50-56: Rate limiter cleanup on re-creation is properly handled.Previous concern about leaked rate limiter intervals has been addressed by destroying the old limiter before creating a new one, and cleaning up in
stopServer.tests/api/server.test.js (1)
1-114: LGTM — clean test suite with good lifecycle coverage.Previous concerns (redundant
vi.unstubAllEnvs(),delete process.envbypassing stub tracking) have been addressed. Using port0for dynamic assignment is a good practice for avoiding port conflicts in tests.src/api/routes/guilds.js (2)
1-11: Good use ofsanitizeMentionsandsafeSendper coding guidelines.Previous critical concern about unsanitized content being sent to Discord has been properly addressed with both
sanitizeMentions()andsafeSend().
93-112: Config scope is clearly documented with issue reference.The global-config-not-per-guild concern from a previous review is now explicitly documented in both the JSDoc and the API response with a reference to Issue
#71.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/routes/guilds.js`:
- Around line 254-257: The long single-line SQL in the dbPool.query call reduces
readability; change the SQL argument passed to dbPool.query (the SELECT from
mod_cases) into a multi-line template literal (backticks) with one column per
line and proper indentation, preserving the explicit column list and the
parameter placeholders ($1, $2, $3) and leaving the parameter array
([req.params.id, limit, offset]) unchanged so the call signature and
functionality of dbPool.query remain the same.
- Around line 312-317: The handler currently calls sanitizeMentions and safeSend
(which may return an array of Message objects when content >2k) and then returns
sent.content from the first chunk; update the endpoint's JSDoc for this
route/handler to explicitly state that safeSend can split messages and the API's
content field will contain only the first chunk when splitting occurs (mention
safeSend and the response shape { id, channelId, content }), so consumers know
the returned content may be truncated and should fetch the full message(s) if
needed.
In `@src/api/server.js`:
- Around line 138-158: The timeout branch of the shutdown Promise leaves the
module-level server reference intact, causing a stale server to remain; update
the timeout handler (the block that uses SHUTDOWN_TIMEOUT_MS and calls
closing.closeAllConnections()) to set server = null before resolving the Promise
so subsequent startServer calls don't see a non-null server and trigger another
stop; keep the call to closing.closeAllConnections() and then set server = null
and resolve() in that order to ensure the server reference is cleared even on
forced timeout.
---
Duplicate comments:
In `@src/api/routes/guilds.js`:
- Around line 178-185: The conversations count was updated to correctly filter
by guild_id; ensure both dbPool.query calls use the same parameter
(req.params.id) as shown and keep the variables conversationResult and
caseResult intact; no additional code changes needed beyond confirming the
SELECT COUNT(*)::int AS count FROM conversations WHERE guild_id = $1 and the
SELECT COUNT(*)::int AS count FROM mod_cases WHERE guild_id = $1 queries use
req.params.id in their respective dbPool.query calls so counts remain scoped to
the guild.
- Around line 205-234: The cursor-based pagination is correctly implemented in
the router.get('/:id/members') handler by using req.guild.members.list({ limit,
after }) and mapping the returned members to memberList; no code changes are
required—leave the current logic (limit clamping, after handling, mapping to
id/username/displayName/roles/joinedAt, and nextAfter calculation) as-is.
In `@src/db.js`:
- Around line 112-130: The ALTER TABLE and CREATE INDEX operations are now each
wrapped in their own try/catch (the pool.query call that adds guild_id and the
pool.query that creates idx_conversations_guild_id), so no further code changes
are required; keep the separate try/catch blocks and the warn(...) calls as-is
and mark the change approved (the previous duplicate comment can be
ignored/removed).
- Remove redundant sanitizeMentions call (safeSend handles it internally) - Document ChannelType enum values in guild info response - Break long SQL query into multi-line template literal - Add comment noting safeSend split behavior for API consumers - Null server reference in shutdown timeout path to prevent stale ref
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/api/routes/guilds.jssrc/api/server.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules withimport/exportsyntax, never userequire()
Always usenode:protocol prefix for Node.js builtin imports (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals, enforced by Biome
Use 2-space indentation throughout the codebase, enforced by Biome
Files:
src/api/server.jssrc/api/routes/guilds.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Never useconsole.log(),console.warn(),console.error(), or any otherconsole.*method in src/ files. Always use Winston logger instead:import { info, warn, error } from '../logger.js'and log with structured metadata
Use custom error classes fromsrc/utils/errors.jsfor error handling. Always log errors with context before re-throwing
UsesplitMessage()utility to handle Discord's 2000-character message limit for outgoing messages
UsesafeSend()wrappers fromsrc/utils/safeSend.jsfor all outgoing messages, and usesanitizeMentions()fromsrc/utils/sanitizeMentions.jsto strip@everyone/@here via zero-width space insertion
Any new code must include tests. Runpnpm testbefore every commit. Maintain minimum 80% code coverage on statements, branches, functions, and lines using@vitest/coverage-v8. PRs that drop coverage below 80% will fail CI
Write JSDoc comments for documentation instead of TypeScript, as the project uses plain JavaScript without TypeScript
Files:
src/api/server.jssrc/api/routes/guilds.js
🧠 Learnings (10)
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Keep documentation files up to date after every code change: update README.md for setup/config changes, update AGENTS.md for architecture/key files changes, update CONTRIBUTING.md for workflow changes, update .env.example for environment variables, update config.json for new config sections
Applied to files:
src/api/server.js
📚 Learning: 2026-02-11T17:18:14.598Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-11T17:18:14.598Z
Learning: See AGENTS.md for full project context, architecture, and coding guidelines
Applied to files:
src/api/server.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/**/*.js : Use custom error classes from `src/utils/errors.js` for error handling. Always log errors with context before re-throwing
Applied to files:
src/api/server.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/**/*.js : Use `safeSend()` wrappers from `src/utils/safeSend.js` for all outgoing messages, and use `sanitizeMentions()` from `src/utils/sanitizeMentions.js` to strip everyone/here via zero-width space insertion
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/**/*.js : Use `splitMessage()` utility to handle Discord's 2000-character message limit for outgoing messages
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/modules/moderation.js : Case numbering must be per-guild sequential and assigned atomically using `COALESCE(MAX(case_number), 0) + 1` in a single INSERT statement inside `createCase()`
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/index.js : Enforce Discord intents: the bot requires MessageContent, GuildMembers, and GuildVoiceStates intents to be enabled in `src/index.js` client setup
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/index.js : The PostgreSQL logging transport is a long-lived Winston transport that requires reactive `onConfigChange` wiring in `src/index.js` startup to add/remove/recreate the transport when `logging.database.*` settings change at runtime
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/commands/**/*.js : Discord timeouts have a maximum duration of 28 days; Discord slowmode has a maximum duration of 6 hours (21600 seconds). Enforce these caps in command logic
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T02:29:07.358Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T02:29:07.358Z
Learning: Applies to src/commands/**/*.js : Moderation commands must follow the pattern: deferReply({ ephemeral: true }), validate inputs, sendDmNotification(), execute Discord action, createCase(), sendModLogEmbed(), checkEscalation(). Use parseDuration() from `src/utils/duration.js` for duration-based commands (timeout, tempban, slowmode)
Applied to files:
src/api/routes/guilds.js
🔇 Additional comments (7)
src/api/routes/guilds.js (3)
73-92: LGTM!Guild info endpoint is clean: caps channel list at 500, documents
ChannelTypeenum values, and returns a focused set of metadata.
206-235: LGTM!Cursor-based pagination using
guild.members.list({ limit, after })is the correct approach for Discord's API. ThenextAftercursor and limit clamping are well implemented.
319-321: The original review comment is accurate.safeSend()does internally sanitize mentions via theprepareOptions() → sanitizeMessageOptions()chain, which in turn callssanitizeMentions()on all text fields (content, embeds, components). The comment correctly documents this behavior. No changes needed.src/api/server.js (4)
11-15: LGTM!Module-scoped
serverandrateLimiterreferences with proper JSDoc type annotations. TherateLimitertracking addresses the prior resource leak concern.
24-78: LGTM!
createAppis well-structured: conditional CORS, rate limiter lifecycle management, clean error handler with appropriate 4xx/5xx differentiation. All prior review feedback has been addressed.
87-117: LGTM!Startup is robust: guards against double-start, validates port (including range), uses
server.once('error')to avoid stale listener issues. All prior feedback incorporated.
124-160: LGTM!Graceful shutdown with timeout fallback, rate limiter cleanup, and proper
server = nullin all paths. The close/timeout race is handled correctly since the promise settles idempotently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/routes/guilds.js`:
- Around line 148-151: The code redundantly calls path.split('.') twice; reuse
the previously computed segments instead of splitting again. Update the code
that currently does const segments = path.split('.') to reuse the existing
segments array (the one used to extract topLevelKey) — e.g., reference the same
variable (segments) when checking for empty segments — so there is only a single
split operation on path and the check for empty segments uses that existing
segments array.
---
Duplicate comments:
In `@src/api/routes/guilds.js`:
- Around line 173-186: The conversation and mod_cases COUNT queries in the
Promise.all are now correctly scoped by guild via dbPool.query('SELECT
COUNT(*)::int AS count FROM conversations WHERE guild_id = $1', ...) and the
corresponding mod_cases query; no code change is required—keep the current
dbPool.query calls (and the explanatory JSDoc about legacy NULL guild_id rows)
as-is to preserve correct scoping and developer context around
conversationResult and caseResult.
- Around line 294-297: The handler branch for action === 'sendMessage' currently
only checks presence of channelId and content but not the type; update the
validation in that branch (the sendMessage check) to ensure channelId is a
string (e.g., typeof channelId === 'string' and non-empty) and return a 400 with
a clear error if it is not, before calling cache.get() or other Discord lookups;
this prevents numeric snowflakes from causing misleading 404s and keeps the
check near the existing channelId/content validation.
| const segments = path.split('.'); | ||
| if (segments.some((s) => s === '')) { | ||
| return res.status(400).json({ error: 'Config path contains empty segments' }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Double path.split('.') call is redundant.
path.split('.') is called on line 137 (to extract topLevelKey) and again on line 148. Consider reusing the result.
♻️ Proposed fix
- const topLevelKey = path.split('.')[0];
+ const segments = path.split('.');
+ const topLevelKey = segments[0];
if (!SAFE_CONFIG_KEYS.includes(topLevelKey)) {
return res.status(403).json({ error: 'Modifying this config key is not allowed' });
}
- if (!path.includes('.')) {
+ if (segments.length < 2) {
return res
.status(400)
.json({ error: 'Config path must include at least one dot separator (e.g., "ai.model")' });
}
- const segments = path.split('.');
if (segments.some((s) => s === '')) {
return res.status(400).json({ error: 'Config path contains empty segments' });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/routes/guilds.js` around lines 148 - 151, The code redundantly calls
path.split('.') twice; reuse the previously computed segments instead of
splitting again. Update the code that currently does const segments =
path.split('.') to reuse the existing segments array (the one used to extract
topLevelKey) — e.g., reference the same variable (segments) when checking for
empty segments — so there is only a single split operation on path and the check
for empty segments uses that existing segments array.
1 participant
Summary
Add Express HTTP server running alongside the Discord WebSocket client, providing a REST API for the upcoming web dashboard.
Closes #29
Endpoints
/api/v1/health/api/v1/guilds/:id/api/v1/guilds/:id/config/api/v1/guilds/:id/config/api/v1/guilds/:id/stats/api/v1/guilds/:id/members/api/v1/guilds/:id/moderation/api/v1/guilds/:id/actionsMiddleware
x-api-secretheader check againstBOT_API_SECRETDASHBOARD_URLoriginStructure
Integration
src/index.jsafter Discord loginBOT_API_PORT,BOT_API_SECRET,DASHBOARD_URLTests
Dependencies
express(already in package.json + lockfile)