feat: AI conversation persistence with DB-backed history#18
feat: AI conversation persistence with DB-backed history#18BillChirico merged 24 commits intomainfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Merge DB results with any messages added concurrently via addToHistory instead of overwriting the cache array. DB messages come first, then any new messages added since the cache miss, trimmed to configured limit.
This comment has been minimized.
This comment has been minimized.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds DB-backed persistent conversation history with in-memory caching, DB schema and cleanup jobs, startup/shutdown pool and AI integration, tooling/config updates (biome, gitignore, config fields), package script tweaks, event/JSDoc improvements, and expanded tests for DB and AI behavior. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@biome.json`:
- Around line 3-13: The includes array in biome.json currently lists only
specific files ("src/db.js","src/index.js","src/modules/ai.js") which leaves out
other source files like src/logger.js, src/modules/config.js,
src/modules/events.js, src/utils/*.js and src/commands/*.js; update the includes
to cover all source files (e.g., use "src/**/*.js") or explicitly add the
missing filenames/globs to the includes list, or if this narrow scope is
intentional add a TODO/comment indicating incremental rollout and which files
will be added later; ensure you update the includes array in biome.json (refer
to the "files" -> "includes" entry) accordingly.
In `@src/db.js`:
- Around line 99-113: The conversations table currently allows any text in the
role column; update the table DDL in src/db.js where the CREATE TABLE query for
conversations is executed to enforce allowed roles by adding a CHECK constraint
(e.g., CHECK (role IN ('user','assistant','system')) ) to the role column
definition, or if the table may already exist, add an ALTER TABLE ... ADD
CONSTRAINT check_role ... clause after the CREATE TABLE to add the CHECK
constraint; ensure the constraint name is unique (e.g.,
check_conversations_role) so the DB will reject invalid role values at the
schema level.
In `@src/index.js`:
- Around line 332-336: The file-based loadState() call is redundant because
initConversationHistory() immediately overwrites in-memory channel state from
DB; remove the loadState() invocation (and any file-only persistence hooks if no
longer needed) so the app relies solely on initConversationHistory() for startup
state, or alternatively modify initConversationHistory() to merge DB records
with existing in-memory state rather than overwriting — locate references to
loadState() and the initConversationHistory() function to either delete the
former and any dependent file persistence code, or implement a merge strategy
inside initConversationHistory() that preserves file-derived channels when DB
records are absent.
In `@src/modules/ai.js`:
- Around line 126-127: Both getHistory and getHistoryAsync are selecting
username in their SQL ("SELECT role, content, username") but the subsequent
mapping only uses role and content; remove the unused column to align with
initConversationHistory. Update the SQL in the getHistory and getHistoryAsync
functions to "SELECT role, content FROM conversations WHERE channel_id = $1" (or
equivalent) so only role and content are fetched, keeping the rest of the
function logic (the .map() that extracts role and content) unchanged.
- Around line 246-289: initConversationHistory currently performs an N+1 query
pattern by fetching distinct channel IDs then querying conversations per
channel; replace the per-channel loop with a single SQL query (using
ROW_NUMBER() PARTITION BY channel_id ORDER BY created_at DESC) executed via
pool.query to retrieve up to getHistoryLength() rows per channel in one
round-trip, then reconstruct the per-channel arrays and populate
conversationHistory (preserving the existing ordering and totalMessages
calculation); locate usages in initConversationHistory, conversationHistory.set,
getPool, getHistoryLength and the existing pool.query calls to implement this
single-query hydration.
- Around line 209-239: The DB write in addToHistory currently fire-and-forgets
and logs failures with logWarn; update the error handling in the
pool.query().catch block to use logError instead of logWarn, include full error
details (err and err.message) and relevant context (channelId, role, username)
so persistence failures are surfaced to alerting; optionally add a simple retry
attempt (e.g., one additional pool.query retry) guarded to avoid blocking the
function, but at minimum replace logWarn with logError in addToHistory and
enrich the logged context (references: addToHistory, conversationHistory,
getPool, pool.query, logWarn/logError).
- Around line 123-140: The inner const limit declared inside the .then()
callback shadows the outer const limit and should be removed; in the callback
that queries conversations (the function that builds dbHistory, current,
merged), delete the redundant "const limit = getHistoryLength()" and use the
previously declared outer limit variable when trimming merged history (e.g.,
when slicing/setting conversationHistory.get/channelId), ensuring
conversationHistory and any concurrent addToHistory logic continue to reference
the same limit value.
- Around line 116-154: getHistory currently sets and returns a new empty array
in conversationHistory but later replaces the map entry with a new array during
async DB hydration, leaving any existing references stale; change the hydration
path in getHistory so it mutates the existing array in-place (get current =
conversationHistory.get(channelId) or create it if missing, compute the merged
slice as now, then replace contents of the existing array via splice/push
instead of conversationHistory.set(... newArray)), ensuring callers holding the
original reference (e.g., code using getHistory, event handlers, addToHistory)
observe the hydrated messages; also keep the same logging (info/logWarn) and
preserve the trim-to-limit behavior used with getHistoryLength().
In `@tests/modules/ai.test.js`:
- Around line 219-285: Add a test exercising the race-condition fix in
initConversationHistory: mock the DB pool query so the first call returns
channel list and the second call returns a Promise you resolve later (capture
resolveDbQuery), call initConversationHistory(), call addToHistory('ch1',
'user', 'concurrent message') while hydration is pending, then resolve the DB
query with a row { role: 'user', content: 'from db' }, await the hydration
promise and assert via getHistory('ch1') that the DB message appears before the
concurrent message and that both messages are present (also assert trimming
behavior if limit applies); use setPool with a mockPool whose query is the
mockQuery and keep references to mockQuery/resolveDbQuery to orchestrate timing.
- Around line 479-493: The test title claims to verify the Authorization header
but only checks Content-Type; update the test for generateResponse to either be
renamed to match the current assertion or (preferred) set the token and assert
Authorization: set process.env.OPENCLAW_TOKEN = 'test-token' (or the actual
env/config key your code reads) before calling generateResponse('ch1', 'Hi',
'user', config), spy globalThis.fetch as done, then after the call assert
fetchCall[1].headers['Authorization'] === 'Bearer test-token' (and you can keep
the Content-Type assertion); also ensure to clean up/reset
process.env.OPENCLAW_TOKEN after the test.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.gitignorebiome.jsonconfig.jsonpackage.jsonsrc/db.jssrc/index.jssrc/modules/ai.jstests/db.test.jstests/modules/ai.test.jsvitest.config.js
🧰 Additional context used
🧠 Learnings (3)
📚 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: 2025-11-26T01:57:34.920Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:57:34.920Z
Learning: Applies to **/*.{ts,tsx,js,jsx,json,css,md} : After changing or editing any files, run the complete validation workflow: `pnpm format && pnpm typecheck && pnpm lint && pnpm build` before committing
Applied to files:
package.jsonbiome.json
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to {package.json,.npmrc,pnpm-lock.yaml} : Use pnpm package manager (pinned to v10.23.0) with `.npmrc` configured for strict peer dependencies and disabled shamefully-hoist
Applied to files:
package.json
🧬 Code graph analysis (4)
src/db.js (1)
src/modules/ai.js (8)
pool(121-121)pool(167-167)pool(171-177)pool(224-224)pool(247-247)pool(262-268)pool(297-297)pool(327-327)
src/index.js (3)
src/utils/registerCommands.js (1)
registerCommands(18-60)src/db.js (3)
client(82-82)closeDb(144-155)initDb(54-127)src/modules/ai.js (3)
stopConversationCleanup(314-320)setPool(83-85)initConversationHistory(246-289)
tests/db.test.js (1)
src/db.js (4)
initDb(54-127)getPool(134-139)closeDb(144-155)pool(12-12)
src/modules/ai.js (3)
src/modules/config.js (5)
getConfig(122-124)pool(58-58)pool(72-72)pool(158-158)pool(228-228)src/db.js (2)
getPool(134-139)pool(12-12)src/logger.js (1)
info(218-220)
⏰ 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: Cursor Bugbot
🔇 Additional comments (14)
.gitignore (1)
20-22: LGTM!Clean addition to ignore coverage output directory, consistent with the new vitest coverage configuration.
vitest.config.js (1)
1-19: LGTM!Clean vitest configuration with sensible coverage thresholds and targeted coverage includes for the files changed in this PR.
tests/db.test.js (1)
1-147: LGTM!Well-structured test suite with proper module isolation via
vi.resetModules(), clean mock setup, and good coverage of the DB module's lifecycle (init, get, close) and schema verification.src/index.js (1)
313-347: Startup sequence is well-ordered.The flow (DB init → config load → pool wiring → file state load → DB hydration → cleanup scheduler) is logically sound. The file-loaded state acts as a fallback for channels without DB records, and DB hydration correctly overwrites on a per-channel basis.
src/modules/ai.js (2)
295-309: Cleanup scheduler captures pool reference at start time.
startConversationCleanupchecksgetPool()on Line 297, butrunCleanupalso callsgetPool()on Line 327 at execution time. This is correctly implemented — if the pool becomes unavailable between scheduler start and a cleanup tick, therunCleanupcall safely returns. Good defensive design.
358-429: History is appended after the API call, creating a brief inconsistency window.Lines 418-419 add the user message and assistant reply to history only after a successful API response. If a second request for the same channel arrives while the first is in-flight, it won't see the first user message in context. For a Discord bot with typical traffic patterns this is largely benign, but worth documenting as a known trade-off.
Otherwise the response flow — async history load, prompt construction, fetch, error fallback — is clean.
package.json (1)
34-37: All package versions are valid and published on npm.config.json (1)
7-9: LGTM!Sensible defaults for conversation history management — 20 messages per channel and a 30-day TTL are reasonable for a Discord bot use case.
tests/modules/ai.test.js (6)
1-53: LGTM — clean test setup with proper isolation.Mock hoisting, per-test state reset, and cleanup timer teardown in
afterEachprovide solid test hygiene.
84-158: Good coverage of addToHistory paths.Trimming, DB write-through, null username, graceful fallback on DB failure, and custom config are all exercised.
160-217: LGTM — getHistoryAsync tests cover the critical paths well.Cache-first retrieval, DB fallback with correct row reversal, empty results, DB errors, and no-pool scenarios are all verified.
287-345: LGTM — cleanup scheduler tests.Good use of
vi.waitForto handle the fire-and-forget async cleanup invocation. Start/stop lifecycle, error handling, and safe idempotent stop are all covered.
368-477: LGTM — generateResponse tests are thorough.Success/error/exception paths, health monitor integration, history updates with username propagation, and default config fallbacks are all well covered.
Also applies to: 495-518
521-546: LGTM — graceful fallback tests.Testing in-memory-only operation and resilience when
getConfigthrows confirms the module degrades safely without a DB or config.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Replace specific file list with src/**/*.js glob for comprehensive coverage - Replace console.log/error with structured logger calls across all source files - Add node: protocol prefix to Node.js builtin imports - Fix unused variables, import ordering, and formatting issues - Apply useTemplate and useExponentiationOperator suggestions
Restrict role values to 'user', 'assistant', 'system' at the database level to prevent invalid data from being persisted.
Document that loadState() is temporary migration scaffolding and is effectively overwritten by initConversationHistory() when DB persistence is available.
- Mutate existing channel history arrays in-place after async hydration to prevent stale references held by callers - Remove shadowed limit variable in getHistory() hydration path - Drop unused username columns from history SELECT queries - Upgrade DB write-through failure logging to logError with channel/role/user context for persistence visibility - Replace startup hydration N+1 query pattern with single ROW_NUMBER() query
- Add regression test for async hydration race where addToHistory occurs while DB hydration is pending; assert in-place array mutation and message ordering - Update initConversationHistory tests for single ROW_NUMBER() hydration query - Align query expectations with SELECT role, content - Rename fetch header test to reflect actual assertion scope
|
Addressed all 10 unresolved review threads in Round 1 ✅ What changed
Validation
All listed review threads were resolved after applying the fixes. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/utils/errors.js (1)
51-56:⚠️ Potential issue | 🟡 MinorPre-existing:
ETIMEDOUTis caught by theNETWORKbranch and never reachesTIMEOUT.Not introduced in this PR, but worth noting: line 51 matches
ETIMEDOUT→NETWORK, so the same check on line 54 is dead code for that code value.TIMEOUTis only reachable via themessage.includes('timeout')fallback. If the intent is forETIMEDOUTto classify asTIMEOUT, the order of these two blocks should be swapped.src/modules/events.js (1)
97-117: 🧹 Nitpick | 🔵 TrivialMissing error handling around
generateResponse— user gets no feedback on failure.If
generateResponseor the subsequentsend/replycalls throw, the error propagates as an unhandled rejection (caught only by the global handler on line 137-138). The user who@mentionedthe bot receives no feedback. Consider wrapping this block in a try/catch that replies with a user-friendly error message (you already havegetUserFriendlyMessageinsrc/utils/errors.js).♻️ Suggested error handling
+ try { await message.channel.sendTyping(); const response = await generateResponse( message.channel.id, cleanContent, message.author.username, config, healthMonitor, ); // Split long responses if (needsSplitting(response)) { const chunks = splitMessage(response); for (const chunk of chunks) { await message.channel.send(chunk); } } else { await message.reply(response); } + } catch (err) { + logError('AI response failed', { channelId: message.channel.id, error: err.message }); + await message.reply('Something went wrong generating a response. Please try again!').catch(() => {}); + } return; // Don't accumulate direct mentions into chime-in buffersrc/modules/ai.js (1)
399-412:⚠️ Potential issue | 🟠 Major
fetchcall to OpenClaw has no timeout — can hang indefinitely.If the OpenClaw API becomes unresponsive, this
fetchwill block forever, tying up the message handler. UseAbortSignal.timeout()to cap the wait.🔧 Suggested fix
const response = await fetch(OPENCLAW_URL, { method: 'POST', headers: { 'Content-Type': 'application/json', ...(OPENCLAW_TOKEN && { Authorization: `Bearer ${OPENCLAW_TOKEN}` }), }, body: JSON.stringify({ model: config.ai?.model || 'claude-sonnet-4-20250514', max_tokens: config.ai?.maxTokens || 1024, messages: messages, }), + signal: AbortSignal.timeout(config.ai?.requestTimeoutMs || 30000), });src/index.js (1)
93-110: 🧹 Nitpick | 🔵 Trivial
saveStateuses synchronous I/O — acceptable for shutdown, but consider the interaction with DB-backed persistence.Once
loadState()is removed (per the migration TODO),saveState()will still serialize the in-memory map to disk on shutdown. After migration, this file becomes a redundant snapshot of data already in PostgreSQL. Track removing both together.
🤖 Fix all issues with AI agents
In `@src/commands/config.js`:
- Around line 286-294: The code assumes setConfigValue returns the section
object (code around path.split('.').slice(1).reduce(...) assigning leafValue
from updatedSection), which will break silently if setConfigValue starts
returning the full root; add a short comment documenting the expected return
shape of setConfigValue and add a runtime guard: after computing leafValue,
check if leafValue === undefined and if so attempt the traversal from the root
(i.e., without .slice(1)) or log a clear warning/error indicating the unexpected
return shape and fall back to using value; reference setConfigValue,
updatedSection, leafValue and path when applying the fix so the traversal and
the warning are updated in the same block.
In `@src/db.js`:
- Around line 99-113: Add a standalone index on created_at to accelerate TTL
cleanup; specifically, alongside the existing CREATE INDEX for
idx_conversations_channel_created, create (if not exists) an index named
idx_conversations_created_at on the conversations table for the created_at
column so the DELETE query used in ai.js (the TTL cleanup: DELETE FROM
conversations WHERE created_at < NOW() - INTERVAL '1 day' * $1) can use an index
instead of a sequential scan.
In `@src/modules/ai.js`:
- Around line 168-206: getHistoryAsync can run duplicate DB queries when
conversationHistory lacks channelId; introduce a lightweight in-flight map
(e.g., pendingHydrations) keyed by channelId to store the promise for the
ongoing hydration so concurrent callers await the same promise. On entry, if
conversationHistory.has(channelId) return it; else if
pendingHydrations.has(channelId) await and return that promise's result;
otherwise create and store a hydration promise that performs the DB query (using
getPool(), getHistoryLength(), querying conversations), on success set
conversationHistory.set(channelId, history) and info(...), on failure log via
logWarn(...) and ensure the promise resolves to an empty array; finally remove
the entry from pendingHydrations before resolving and return the cached history.
In `@src/modules/welcome.js`:
- Around line 98-100: The catch block currently logs only err.message which
loses the stack; update the catch handler where logError('Welcome error', ...)
is called in src/modules/welcome.js to include the full stack (e.g., add stack:
err.stack or pass the entire err object) in the metadata so the error stack
trace is preserved when calling logError.
In `@tests/modules/ai.test.js`:
- Around line 165-173: The test should also assert that the write-failure is
logged: update the test around addToHistory to mock the logger used by the
module (e.g., console.error or the module's logger) before calling
setPool/addToHistory, trigger the rejected query as already done, and assert the
logger was called with the expected message that includes the write failure
context (channel id 'ch1' and role 'user' or whatever the implementation logs);
adjust the expected log text to match the exact string/format produced by the
addToHistory implementation.
- Around line 83-119: Replace the fragile single microtick flush (the `await
Promise.resolve()` after awaiting hydrationPromise) with a robust wait that
retries until the expected state is observed; use vi.waitFor to await the
condition on historyRef/getHistory('race-channel') instead of relying on one
microtick. Specifically, after resolving hydrationPromise, call vi.waitFor with
an assertion that historyRef equals the expected array (or that
getHistory('race-channel') returns historyRef) so the test waits for any
additional async work triggered by mockQuery/hydrationPromise to complete.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
biome.jsonsrc/commands/config.jssrc/commands/ping.jssrc/commands/status.jssrc/db.jssrc/index.jssrc/logger.jssrc/modules/ai.jssrc/modules/chimeIn.jssrc/modules/config.jssrc/modules/events.jssrc/modules/spam.jssrc/modules/welcome.jssrc/utils/errors.jssrc/utils/registerCommands.jssrc/utils/retry.jstests/modules/ai.test.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-26T01:57:34.920Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:57:34.920Z
Learning: Applies to **/*.{ts,tsx,js,jsx,json,css,md} : After changing or editing any files, run the complete validation workflow: `pnpm format && pnpm typecheck && pnpm lint && pnpm build` before committing
Applied to files:
biome.json
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to src/**/*.{ts,tsx} : Use `reportError(context, error)` from `src/lib/logger.ts` to report errors to Sentry with context metadata, falling back to console.error if Sentry is disabled
Applied to files:
src/modules/welcome.jssrc/logger.js
🧬 Code graph analysis (9)
src/utils/registerCommands.js (1)
src/logger.js (1)
info(215-217)
src/modules/welcome.js (1)
src/logger.js (1)
info(215-217)
src/index.js (3)
src/db.js (3)
client(82-82)closeDb(144-155)initDb(54-127)src/modules/ai.js (4)
stopConversationCleanup(330-336)setPool(83-85)initConversationHistory(255-305)startConversationCleanup(311-325)src/logger.js (2)
info(215-217)error(229-231)
src/db.js (1)
src/modules/ai.js (8)
pool(121-121)pool(174-174)pool(178-184)pool(231-231)pool(256-256)pool(266-276)pool(313-313)pool(343-343)
src/modules/events.js (1)
src/logger.js (2)
info(215-217)warn(222-224)
src/modules/ai.js (3)
src/logger.js (2)
config(27-27)info(215-217)src/modules/config.js (5)
getConfig(130-132)pool(58-58)pool(74-74)pool(166-166)pool(243-243)src/db.js (2)
getPool(134-139)pool(12-12)
src/commands/config.js (1)
src/modules/config.js (2)
section(150-150)err(30-30)
tests/modules/ai.test.js (4)
src/modules/ai.js (6)
OPENCLAW_URL(104-107)OPENCLAW_URL(104-107)OPENCLAW_TOKEN(108-108)OPENCLAW_TOKEN(108-108)config(31-31)config(46-46)src/modules/config.js (2)
getConfig(130-132)i(360-360)src/logger.js (2)
warn(222-224)info(215-217)src/index.js (1)
config(50-50)
src/modules/config.js (4)
src/commands/config.js (4)
value(268-268)section(189-189)section(271-271)section(321-321)src/index.js (1)
client(53-61)src/db.js (1)
client(82-82)src/logger.js (1)
info(215-217)
⏰ 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: Cursor Bugbot
🔇 Additional comments (28)
src/utils/errors.js (1)
118-156: Formatting-only changes look good.The reformatted message map entries are cleaner and consistent with the project's new Biome formatting rules.
src/modules/spam.js (1)
27-51: Formatting-only changes — LGTM.Arrow function parentheses, multi-line
.fetch(), lowercase hex color, and trailing comma are all consistent with the new Biome formatting rules. No behavioral impact.src/modules/chimeIn.js (2)
12-14: Import reordering — LGTM.
42-61: Formatting-only changes throughout — LGTM.Object literal restructuring, single-line
.join()calls, and sort callback reformatting are all cosmetic. No behavioral impact.Also applies to: 92-92, 149-149
src/commands/ping.js (1)
7-17: Formatting-only changes — LGTM.Trailing comma and single-line template string are consistent with Biome config. No behavioral impact.
src/modules/events.js (2)
6-11: Logging and utility imports look good.Clean migration from
console.*to structured logger functions, and proper import of message-splitting utilities.
132-140: Error handlers properly migrated to structured logging — LGTM.Defensive
err?.message || String(err)handles non-Error rejections. Including stack traces aids debugging.src/commands/config.js (3)
76-114: Well-structured recursive path collector — LGTM.
collectConfigPathscorrectly handles leaf values, nested objects, arrays (including empty ones), and empty objects. The accumulator pattern avoids unnecessary allocations.
218-251: Good addition: embed size tracking prevents Discord API errors.Tracking cumulative field length against
EMBED_CHAR_LIMIT - 200and adding a truncation notice is a solid defensive pattern for avoiding Discord's 6000-character embed limit.
14-16: Nice defensive addition for inline code injection.
escapeInlineCodeprevents user-supplied section names from breaking Discord's backtick formatting — good practice for any user-controlled string embedded in markdown.biome.json (1)
1-34:⚠️ Potential issue | 🔴 CriticalUpdate schema version to 2.3.11 — the version referenced (2.3.14) does not exist. The latest Biome CLI is 2.3.11 as of February 2026. Update the
$schemaURL to"https://biomejs.dev/schemas/2.3.11/schema.json".The rest of the configuration is well-structured: the
src/**/*.jsandtests/**/*.jsglobs cover all source and test files, thenoConsole: "error"rule is appropriate given the centralized logger usage, and the formatter settings are consistent with the changes in this PR.⛔ Skipped due to learnings
Learnt from: CR Repo: VolvoxCommunity/Volvox.Website PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-26T01:57:34.920Z Learning: Applies to **/*.{ts,tsx,js,jsx,json,css,md} : After changing or editing any files, run the complete validation workflow: `pnpm format && pnpm typecheck && pnpm lint && pnpm build` before committingsrc/logger.js (2)
11-13: Good modernization of imports tonode:protocol.Using
node:fs,node:path, andnode:urlprefixes is the recommended practice for Node.js built-in modules in ESM, making it explicit these are core modules.
102-102: Good switch toObject.hasOwn.
Object.hasOwnis safer and more readable thanhasOwnPropertychecks.src/utils/retry.js (1)
1-130: Formatting and style-only changes look good.Import reorder, exponentiation operator, and arrow function formatting are all clean improvements with no behavioral impact.
src/utils/registerCommands.js (1)
1-59: Clean migration to structured logging with proper error propagation.The error is logged and re-thrown, preserving the stack for callers. Formatting and REST call restructuring look correct.
src/modules/config.js (3)
26-40:loadConfigFromFilecaches on first successful parse — subsequent changes toconfig.jsonrequire a restart.This is documented by the comment on line 18 ("loaded once, never invalidated"), which is appropriate for a seed/fallback file. The implementation is clean.
96-105: Transaction handling is correct with proper cleanup.ROLLBACK in catch,
client.release()in finally — this pattern is repeated consistently throughout the file. The/* ignore rollback failure */comments are acceptable since the original error is re-thrown.
332-346: Good prototype pollution protection.Checking
__proto__,constructor, andprototypein bothvalidatePathSegmentsandsetNestedValueprovides defense-in-depth against path-based prototype pollution attacks on config mutation.src/commands/status.js (1)
139-152: Robust error handling with deferred/replied state awareness.The pattern of checking
interaction.replied || interaction.deferredbefore choosingfollowUpvsreplycorrectly handles all interaction states. The.catch(() => {})swallows any secondary failure, which is appropriate for a last-resort error handler.src/modules/ai.js (2)
116-161: Previous stale-reference and shadowed-variable issues have been addressed.The in-place mutation pattern (
arr.length = 0; arr.push(...)) on lines 143-144 correctly preserves references for callers, and the redundantlimitdeclaration has been removed. The unusedusernamecolumn has also been dropped from the query.
239-246: Write-through failure now logs atlogErrorlevel — previous concern addressed.The upgrade from
logWarntologError(line 240) properly surfaces persistence failures for alerting.src/index.js (2)
232-274: Shutdown sequencing is well-ordered.The sequence — drain pending requests → stop cleanup timer → save state → close DB → destroy client — is correct and avoids race conditions between the cleanup scheduler and DB pool teardown.
313-349: Startup sequence is clean with proper dependency ordering.DB init → config load → pool injection → state hydration → event registration → login. The
loadState()migration TODO is acknowledged. One observation:startConversationCleanup()(line 341) is called unconditionally but guards internally against missing DB — this is fine but could be guarded here for clarity.tests/modules/ai.test.js (5)
1-53: Clean test scaffolding with proper isolation.State reset in
beforeEach(new Map, null pool, clear mocks) andafterEach(stop cleanup, restore mocks) ensures good test isolation. The mock defaults align with the documented config defaults.
198-255: Solid async history retrieval coverage.Cache-first, DB fallback with reversal, empty results, error logging, and no-pool scenarios are all well covered. The assertion on the LIMIT parameter (line 224) correctly validates the config integration.
257-319: Good hydration test coverage validating the N+1 fix.Line 288–289 correctly asserts a single query using
ROW_NUMBER(), confirming the optimized hydration path. Error and empty-result edge cases are covered.
321-379: Cleanup scheduler tests are well-structured.Good use of
vi.waitForfor async assertions and properstopConversationCleanup()teardown in each test.
555-580: Good fallback coverage ensuring resilience when config is unavailable.The
getConfigthrowing test (line 568) validates the defaulthistoryLengthof 20 is applied, which is critical for the graceful degradation path.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/modules/events.js (1)
149-155: 🧹 Nitpick | 🔵 TrivialJSDoc
@paramtype forclientnot updated inregisterEventHandlers.The other handler functions were updated to
@param {Client} client, but this one still says@param {Object} client.🧹 Proposed fix
/** * Register all event handlers - * `@param` {Object} client - Discord client + * `@param` {Client} client - Discord client * `@param` {Object} config - Bot configuration * `@param` {Object} healthMonitor - Health monitor instance */src/index.js (1)
273-287: 🧹 Nitpick | 🔵 TrivialDuplicate
errorandunhandledRejectionhandlers withevents.js.
client.on('error', ...)(Line 273) duplicatesclient.on(Events.Error, ...)insrc/modules/events.jsLine 137 — both fire on every client error. TheunhandledRejectionhandler (Line 281) is mitigated by theprocessHandlersRegisteredguard inevents.js, but the client error handler has no such guard.Consider consolidating error handling in one location to avoid double-logging.
tests/modules/ai.test.js (1)
674-895:⚠️ Potential issue | 🔴 CriticalRemove this entire duplicate test suite — it breaks CI.
Lines 674–895 are a stale copy of the old test suite appended after the new one. This causes the pipeline failures:
noRedeclare:afterEach,beforeEach,describe, etc. are re-importedorganizeImports: the second import block is unsorted- It also re-invokes
vi.mock('../../src/logger.js')and imports the removedgetHistoryAPIDelete everything from line 674 onward.
🔧 Proposed fix
-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; - -// Mock logger -vi.mock('../../src/logger.js', () => ({ - info: vi.fn(), - error: vi.fn(), - warn: vi.fn(), - debug: vi.fn(), -})); - -import { - addToHistory, - generateResponse, - getConversationHistory, - getHistory, - OPENCLAW_TOKEN, - OPENCLAW_URL, - setConversationHistory, -} from '../../src/modules/ai.js'; - -describe('ai module', () => { - ... (all lines 674-895) -});Remove lines 674 through 895 entirely.
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 21-22: Remove the redundant duplicate entry "coverage/" from
.gitignore (the duplicate found between lines shown in the diff); keep the
original listing and delete the second "coverage/" line so the file only
contains one entry for coverage.
In `@src/modules/ai.js`:
- Around line 347-368: The runCleanup function currently catches DB errors using
logWarn which is inconsistent with addToHistory's error-level logging; update
the catch block in runCleanup to use the same error-level logger used by
addToHistory (replace logWarn('Conversation cleanup failed', { error:
err.message }) with the error-level logging call used elsewhere, include the
error details and same context keys), keeping getPool, getHistoryTTLDays and the
surrounding try/catch intact.
In `@tests/db.test.js`:
- Line 177: The file contains duplicated import declarations for the Vitest
helpers (afterEach, beforeEach, describe, expect, it, vi) — remove the redundant
import block and the superseded first test suite so the helpers are only
imported once (or alternatively split the two suites into separate test files);
locate the duplicate import of afterEach/beforeEach/describe/expect/it/vi and
the surrounding first suite and delete that whole initial suite (or move it into
its own file) so lint noRedeclare and import-sorting errors are resolved.
In `@tests/modules/ai.test.js`:
- Around line 69-125: Tests define duplicate describe('getHistoryAsync') blocks
which should be merged to avoid confusing duplicate suite names; locate the two
describe('getHistoryAsync') suites and consolidate their it(...) cases into a
single describe('getHistoryAsync') block (keeping tests: "should create empty
history for new channel", "should return existing history for known channel",
and the concurrency/hydration test that uses getHistoryAsync,
getConversationHistory, setPool, addToHistory and vi mocks) then remove the
redundant describe block so all related it tests run under one suite and
maintain shared setup/teardown if any.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.gitignoreconfig.jsonpackage.jsonsrc/db.jssrc/index.jssrc/modules/ai.jssrc/modules/events.jstests/db.test.jstests/index.test.jstests/modules/ai.test.js
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM syntax withimport/export; never userequire()
Always usenode:protocol when importing Node.js builtins (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston logging viaimport { info, warn, error } from '../logger.js'— NEVER useconsole.log(),console.warn(),console.error(), or anyconsole.*method in src/ files
Pass structured metadata when logging:info('Message processed', { userId, channelId })
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig()fromsrc/modules/config.jsto read config; usesetConfigValue(key, value)to update at runtime
UsesplitMessage()utility for messages exceeding Discord's 2000-character limit
No TypeScript — use plain JavaScript with JSDoc comments for documentation
Files:
src/modules/events.jssrc/db.jssrc/index.jssrc/modules/ai.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Check
config.yourModule.enabledbefore processing in module handlers
Files:
src/modules/events.jssrc/modules/ai.js
tests/**/*.{js,test.js}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for testing with
pnpm test; test coverage provider is@vitest/coverage-v8
Files:
tests/db.test.jstests/index.test.jstests/modules/ai.test.js
🧠 Learnings (10)
📚 Learning: 2026-02-11T17:18:29.603Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Module handlers must be registered in `src/modules/events.js` to wire them to Discord events
Applied to files:
src/modules/events.js
📚 Learning: 2026-02-11T17:18:29.603Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Graceful shutdown is handled in `src/index.js`; use error classes and logging before throwing errors in other modules
Applied to files:
src/modules/events.jssrc/index.js
📚 Learning: 2026-02-11T17:18:29.603Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Applies to src/**/*.js : Use `splitMessage()` utility for messages exceeding Discord's 2000-character limit
Applied to files:
src/modules/events.js
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to {package.json,.npmrc,pnpm-lock.yaml} : Use pnpm package manager (pinned to v10.23.0) with `.npmrc` configured for strict peer dependencies and disabled shamefully-hoist
Applied to files:
package.json
📚 Learning: 2026-02-11T17:18:29.603Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Applies to tests/**/*.{js,test.js} : Use Vitest for testing with `pnpm test`; test coverage provider is `vitest/coverage-v8`
Applied to files:
package.jsontests/db.test.js.gitignore
📚 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-11T17:18:29.603Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Commands are auto-discovered from `src/commands/` on startup; after adding a command, run `pnpm run deploy` to register with Discord
Applied to files:
package.json
📚 Learning: 2025-11-26T01:57:34.920Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:57:34.920Z
Learning: Applies to **/*.{ts,tsx,js,jsx,json,css,md} : After changing or editing any files, run the complete validation workflow: `pnpm format && pnpm typecheck && pnpm lint && pnpm build` before committing
Applied to files:
package.json
📚 Learning: 2026-02-11T17:18:29.603Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Applies to src/**/*.js : Use single quotes for strings (enforced by Biome)
Applied to files:
package.json
📚 Learning: 2026-02-11T17:18:29.603Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Applies to src/**/*.js : Use 2-space indentation (enforced by Biome)
Applied to files:
package.json
🧬 Code graph analysis (5)
src/modules/events.js (1)
src/index.js (1)
client(54-62)
tests/db.test.js (1)
src/db.js (4)
initDb(54-132)getPool(139-144)closeDb(149-160)pool(12-12)
tests/modules/ai.test.js (1)
src/modules/config.js (2)
getConfig(130-132)i(360-360)
src/db.js (1)
src/modules/config.js (4)
pool(58-58)pool(74-74)pool(166-166)pool(243-243)
src/index.js (1)
src/modules/ai.js (4)
stopConversationCleanup(335-341)setPool(86-88)initConversationHistory(249-309)startConversationCleanup(315-330)
🪛 GitHub Actions: CI
tests/db.test.js
[error] 1-1: assist/source/organizeImports: The imports and exports are not sorted. Safe fix: Organize Imports (Biome).
[error] 177-177: lint/suspicious/noRedeclare: Shouldn't redeclare 'afterEach'.
[error] 177-177: lint/suspicious/noRedeclare: Shouldn't redeclare 'beforeEach'.
[error] 177-177: lint/suspicious/noRedeclare: Shouldn't redeclare 'describe'.
[error] 177-177: lint/suspicious/noRedeclare: Shouldn't redeclare 'expect'.
[error] 177-177: lint/suspicious/noRedeclare: Shouldn't redeclare 'it'.
[error] 177-177: lint/suspicious/noRedeclare: Shouldn't redeclare 'vi'.
tests/modules/ai.test.js
[error] 674-674: assist/source/organizeImports: The imports and exports are not sorted. Safe fix: Organize Imports (Biome).
[error] 674-674: lint/suspicious/noRedeclare: Shouldn't redeclare 'afterEach'.
⏰ 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: Cursor Bugbot
🔇 Additional comments (16)
config.json (1)
7-9: LGTM!New config fields
historyLengthandhistoryTTLDayswith sensible defaults that match the fallback constants insrc/modules/ai.js.src/db.js (1)
54-131: LGTM!The reordered guard logic, conversations table schema with CHECK constraint, composite index, and standalone
created_atindex for TTL cleanup are all sound. The error path properly cleans up the pool on schema creation failure.package.json (1)
1-38: LGTM!Script additions (
test:watch) and dependency updates look reasonable.src/modules/events.js (1)
6-6: LGTM — UsingEventsconstants instead of string literals.Good improvement for type safety and refactoring resilience.
src/modules/ai.js (6)
120-183: Hydration logic is well-structured with proper deduplication and in-place mutation.The
pendingHydrationsmap prevents duplicate DB queries for the same channel, the in-place array mutation preserves references for concurrent callers, and the merge ordering (DB first, then in-flight writes) is correct. Previous review concerns about shadowed variables, unusedusernamecolumn, N+1 queries, and concurrent hydration deduplication have all been addressed.
224-241: Fire-and-forget DB write now logs aterrorlevel with context — good.Previous review concern about
warnvserrorlevel for persistence failures has been addressed.
249-309: LGTM — Single-query hydration withROW_NUMBER()window function.Addresses the previous N+1 query concern. The TTL filter in the subquery avoids scanning expired rows, and in-place replacement prevents duplicate growth across restarts.
315-330: LGTM —cleanupTimer.unref()applied as previously suggested.
438-440: User message content is prefixed with username in history.
addToHistory(channelId, 'user',${username}: ${userMessage}, username)stores"username: message"as the content. When this is hydrated from DB and then sent back to the API (Line 399), the model will see"username: message"in the history — which is the intended format matching the current message on Line 399. This is consistent.
86-88: No issues found. ThesetPoolfunction at lines 86-88 is correctly implemented with the parameter name matching the assignment (pool→_poolRef = pool).src/index.js (2)
304-332: Startup sequence is well-ordered and handles the no-DB fallback gracefully.The flow correctly: initializes DB → loads config → sets pool on AI module → loads file state (migration path) → hydrates from DB (overwrites file state) → starts cleanup → registers events → loads commands → logs in. The TODO on Lines 323-325 clearly documents the migration intent.
244-266: Shutdown sequence correctly stops cleanup before saving state and closing DB.The ordering ensures no cleanup queries run against a closed pool, and state is persisted before the pool is torn down.
tests/db.test.js (1)
222-369: Second test suite (Lines 222–369) is comprehensive and well-structured.Good coverage of init, idempotency, concurrent rejection, getPool, closeDb (including error handling), schema validation, and SSL configuration variants. The class-based
Poolmock properly captures config for SSL assertions. The re-entrancy guard test and connection failure cleanup test are good additions addressing prior review feedback.tests/index.test.js (1)
31-34: LGTM — mock additions are consistent and complete.New AI lifecycle mocks (
setPool,initConversationHistory,startConversationCleanup,stopConversationCleanup) are properly declared, wired intovi.mock, and reset inimportIndex. Clean and symmetric with existing patterns.Also applies to: 126-129, 194-197
tests/modules/ai.test.js (2)
82-124: Good coverage of the hydration race condition.This test properly exercises the concurrent-write-during-hydration scenario and uses
vi.waitForfor resilience, addressing prior feedback.
170-190: Good — DB write failure now asserts error logging with context.Addresses the prior feedback about verifying that write-through failures are logged with structured context (
channelId,role,username,error).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Test and coverage outputs | ||
| coverage/ |
There was a problem hiding this comment.
Duplicate coverage/ entry.
coverage/ is already listed on Line 5. This second entry is redundant.
🧹 Proposed fix
-# Test and coverage outputs
-coverage/
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Test and coverage outputs | |
| coverage/ |
🤖 Prompt for AI Agents
In @.gitignore around lines 21 - 22, Remove the redundant duplicate entry
"coverage/" from .gitignore (the duplicate found between lines shown in the
diff); keep the original listing and delete the second "coverage/" line so the
file only contains one entry for coverage.
| async function runCleanup() { | ||
| const pool = getPool(); | ||
| if (!pool) return; | ||
|
|
||
| try { | ||
| const ttlDays = getHistoryTTLDays(); | ||
| const result = await pool.query( | ||
| `DELETE FROM conversations | ||
| WHERE created_at < NOW() - INTERVAL '1 day' * $1`, | ||
| [ttlDays], | ||
| ); | ||
|
|
||
| if (result.rowCount > 0) { | ||
| info('Cleaned up old conversation messages', { | ||
| deleted: result.rowCount, | ||
| ttlDays, | ||
| }); | ||
| } | ||
| } catch (err) { | ||
| logWarn('Conversation cleanup failed', { error: err.message }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cleanup failure logged at warn — consider error for consistency.
addToHistory DB failures are logged at error level (Line 234), but runCleanup DB failures use logWarn (Line 366). Both represent persistence-layer failures; using the same severity would be more consistent for alerting.
🤖 Prompt for AI Agents
In `@src/modules/ai.js` around lines 347 - 368, The runCleanup function currently
catches DB errors using logWarn which is inconsistent with addToHistory's
error-level logging; update the catch block in runCleanup to use the same
error-level logger used by addToHistory (replace logWarn('Conversation cleanup
failed', { error: err.message }) with the error-level logging call used
elsewhere, include the error details and same context keys), keeping getPool,
getHistoryTTLDays and the surrounding try/catch intact.
Resolve six issues identified during review of the last five merged PRs: - Wrap Discord message sends in try-catch with user-friendly fallback - Invert NODE_ENV check to hide error details by default - Remove duplicate unhandledRejection handler from index.js - Remove dead graceful-shutdown request tracking code - Null out ChimeIn AbortController in finally block - Add throttled eviction to welcome activity Map
Resolve six issues identified during review of the last five merged PRs: - Wrap Discord message sends in try-catch with user-friendly fallback - Invert NODE_ENV check to hide error details by default - Remove duplicate unhandledRejection handler from index.js - Remove dead graceful-shutdown request tracking code - Null out ChimeIn AbortController in finally block - Add throttled eviction to welcome activity Map
- Test refreshDiscordToken: success, failure, token rotation, no refresh token (issue #15) - Test fetchWithRateLimit: 429 response, retry-after parsing, max retries exhaustion (issue #16) - Test proxy: actual redirect behavior for unauthenticated requests, passthrough for valid tokens, callbackUrl preservation (issue #17) - Fix mock types: complete Account/Token objects with all required fields (issue #18) - Update guilds API test to use getToken() instead of session.accessToken - Update landing page tests for conditional invite button rendering - Update header/dashboard-shell/server-selector tests for refactored components - Add health check endpoint test - All 82 tests passing, lint clean, build succeeds
… (#83) * feat(dashboard): add Discord entity pickers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(dashboard): add diff view and polish - ConfigDiff.tsx: visual diff component showing before/after config changes with green additions and red deletions using the `diff` library - SystemPromptEditor.tsx: textarea with real-time character count, max length warning indicator, and accessible labeling - Toast notifications via sonner: success/error toasts on save, load failures, and reset actions positioned bottom-right - ResetDefaultsButton with confirmation dialog using Radix UI Dialog - ConfigEditor.tsx: full config editing page with AI, welcome message, and moderation sections; PATCH-based save with diff preview - Config API proxy route (GET/PATCH) following established analytics proxy pattern with guild admin authorization - Dialog UI component (shadcn/ui new-york style) - Added lint script to web/package.json Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * polish(config-editor): improve UX, accessibility, and edge case handling - Replace native checkboxes with styled toggle switches using proper role="switch" and aria-checked attributes - Add unsaved changes guard (beforeunload warning + yellow banner) - Add Ctrl/Cmd+S keyboard shortcut for saving - Block save when system prompt exceeds character limit - Rename misleading "Reset to Defaults" to "Discard Changes" with accurate dialog copy (reverts to last saved state, not factory defaults) - Add diff summary counts (+N / -N) to the pending changes card - Improve accessibility throughout: aria-labels on loading spinner, aria-describedby linking textareas to their hints, aria-invalid on over-limit prompt, aria-live on character counter, aria-hidden on decorative icons, role="region" on diff view - Memoize hasChanges and hasValidationErrors to avoid redundant JSON.stringify on every render - Validate PATCH body shape in API route before proxying upstream - Fix stale "bills-bot" prefix in guild-selection localStorage keys (missed during volvox rename) Closes #31 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(api): add config endpoints for Issue #31 Add REST API endpoints for managing bot configuration: - GET /api/v1/config - Retrieve current config (ai, welcome, spam, moderation) - PUT /api/v1/config - Update config with schema validation Features: - Type-safe schema validation for config sections - Flatten nested objects to dot-notation paths for persistence - requireGlobalAdmin middleware (API-secret or bot-owner OAuth) - Proper HTTP error codes (400 for validation, 401/403 for auth, 500 for errors) - Added PUT to CORS methods Tests: - 35 comprehensive tests covering auth, validation, types, edge cases - Tests for validateConfigSchema and flattenToLeafPaths exports Closes #31 * feat(api): add webhook notifications for config changes - Add notifyDashboardWebhook() fire-and-forget sender to PATCH /:id/config - POST /webhooks/config-update endpoint for dashboard to push config changes - Webhook uses DASHBOARD_WEBHOOK_URL env var with 5s timeout - Add comprehensive tests for webhook functionality * feat(dashboard): add config editor with Zustand state management Add a full config editor UI at /dashboard/config with: - Proxy API routes (GET + PATCH) for bot config at /api/guilds/:guildId/config - Zustand store for config state with fetch, update, and debounced saves - Accordion-based sections for ai, welcome, spam, moderation (read-only), triage - Recursive field renderer supporting booleans, numbers, strings, arrays, objects - shadcn/ui components: accordion, input, label, switch, textarea Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(dashboard): enhance Discord entity pickers with multi-select and Zustand - Add multi-select mode to ChannelSelector and RoleSelector (multiple?: boolean prop) - Create Zustand store for caching channels/roles per guild - Add dedicated bot API endpoints: GET /:id/channels and GET /:id/roles - Add Next.js proxy routes for channels and roles - Update AGENTS.md with new key files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove unused import in webhooks.js * fix: resolve all linting errors across codebase - Migrate biome config from 2.3.14 to 2.4.0 - Fix unused imports (triage.js, modAction.test.js) - Fix import ordering (auth.js, lock.js, unlock.js, ai.js, triage-respond.js, modAction.js, modAction.test.js) - Fix formatting across 19 files - Replace O(n²) spread in reduce with push (cli-process.test.js) - Use Object.hasOwn() instead of Object.prototype.hasOwnProperty (config-guild.test.js) All 1310 tests pass. * feat: add full config editing support for moderation and triage - Add moderation and triage to SAFE_CONFIG_KEYS in guilds.js, webhooks.js, and config.js making them writable via PATCH/PUT endpoints - Expand READABLE_CONFIG_KEYS to include all sections: ai, welcome, spam, moderation, triage, logging, memory, permissions - Add CONFIG_SCHEMA definitions for moderation and triage sections with full type validation - Update WritableConfigSection type to include moderation and triage - Remove moderation from READ_ONLY_SECTIONS in config-section.tsx - Update config-store.ts writable keys check - Add editable moderation section in dashboard config-editor with toggles for enabled, autoDelete, DM notifications, and escalation - Add editable triage section with fields for models, budgets, intervals, streaming, debug footer, and moderation log channel - Update all test assertions to reflect new writable sections Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): add webhook URL validation, schema validation on all write paths, atomic writes - Add shared validateWebhookUrl() utility that blocks SSRF (localhost, private IPs, link-local, IPv6 loopback) and enforces https in production - Wire URL validation into config.js notifyWebhook and guilds.js notifyDashboardWebhook - Export validateSingleValue() from config.js and apply it to the PATCH endpoint in guilds.js and the POST /config-update webhook endpoint - Add path length (<=200 chars) and depth (<=10 segments) limits to guilds.js PATCH and webhooks.js POST endpoints - Refactor PUT handler in config.js to track per-field write results: returns 200 on full success, 207 on partial failure, 500 when all fail - Add comprehensive tests for all new validations and error responses Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: extract shared config allowlist, webhook utility, and proxy helpers; remove dead code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(frontend): batch saves, fix race conditions, DRY constants, localStorage safety - Batch saveChanges into parallel PATCH requests grouped by section instead of sequential individual PATCHes (I5) - Add request sequence counter to Zustand config store to prevent stale PATCH responses from clobbering newer state (I6) - Centralize SYSTEM_PROMPT_MAX_LENGTH constant in types/config.ts and import in config-editor and system-prompt-editor (M2) - Wrap localStorage.getItem in try/catch for SSR safety (M3) - Fix channels.length / roles.length truthiness bug — use !== undefined instead of .length which is falsy for 0 (M5) - Replace JSON.stringify deep comparison with recursive deepEqual utility function (M8) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): mask sensitive config fields, block IPv4-mapped IPv6 SSRF, reject unknown config paths - Add SENSITIVE_FIELDS set and maskSensitiveFields utility to strip triage API keys (classifyApiKey, respondApiKey) from all GET config responses - Block SSRF via IPv4-mapped IPv6 addresses (::ffff:127.0.0.1, hex form ::ffff:7f00:1, and cloud metadata ::ffff:169.254.169.254) - Reject unknown config paths in validateSingleValue instead of silently accepting them without type checking - Add cache size limit (100 entries) to webhook URL validation cache - Guard flattenToLeafPaths against __proto__/constructor/prototype keys Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(backend): extract shared validators and getBotOwnerIds, add webhook utility tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(frontend): remove dead code, fix save flow, harden inputs and type guards - C4: delete 10 unused files (stores, UI components, dashboard selectors) and remove zustand, @radix-ui/react-accordion, @radix-ui/react-label, @radix-ui/react-switch from package.json - m8: replace ~85-line local GuildConfig interface with DeepPartial<BotConfig> (add DeepPartial utility to types/config.ts) - m4: harden isGuildConfig type guard to verify at least one expected section key (ai, welcome, spam) instead of just typeof === "object" - M6: fix computePatches to include top-level paths (remove incorrect fullPath.includes(".") guard that silently dropped top-level field changes) - M7: fix partial save to merge only succeeded sections into savedConfig on partial failure, preserving draft edits for failed sections; only call fetchConfig() on full success - m5: add min constraints to triage number inputs (budgets min=0, timeouts min=1, buffer/context sizes min=1) - m9: add e.returnValue = "" to beforeunload handler for modern browser support Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: fix 10-segment test to use valid schema path after strict validation * fix: wire default-personality.md as system prompt fallback Replace generic 'You are a helpful Discord bot.' fallback with the existing default-personality.md template, which provides the intended Volvox Bot personality, role constraints, and anti-abuse guardrails. * fix: merge default-personality into responder system prompt, remove dead template Merge personality, role, constraints, and anti-abuse from the unused default-personality.md into triage-respond-system.md (the responder's actual system prompt). Revert the fallback wiring in triage-prompt.js since personality now lives in the system prompt file where it belongs. Delete default-personality.md — no longer needed. * 📝 Add docstrings to `feat/config-editor-combined` Docstrings generation was requested by @BillChirico. The following files were modified: * `src/api/routes/auth.js` * `src/api/routes/config.js` * `src/api/routes/guilds.js` * `src/api/utils/configAllowlist.js` * `src/api/utils/validateConfigPatch.js` * `src/api/utils/validateWebhookUrl.js` * `src/commands/lock.js` * `src/commands/slowmode.js` * `src/commands/unlock.js` * `src/modules/memory.js` * `src/modules/triage-prompt.js` * `src/modules/triage-respond.js` * `src/modules/triage.js` * `src/utils/debugFooter.js` * `src/utils/permissions.js` * `web/src/app/api/guilds/[guildId]/channels/route.ts` * `web/src/app/api/guilds/[guildId]/config/route.ts` * `web/src/app/api/guilds/[guildId]/roles/route.ts` * `web/src/app/dashboard/config/page.tsx` * `web/src/components/dashboard/config-diff.tsx` * `web/src/components/dashboard/config-editor.tsx` * `web/src/components/dashboard/reset-defaults-button.tsx` * `web/src/components/dashboard/system-prompt-editor.tsx` * `web/src/components/providers.tsx` * `web/src/lib/bot-api-proxy.ts` These files were kept as they were: * `src/api/server.js` * `src/api/utils/webhook.js` These files were ignored: * `tests/api/routes/config.test.js` * `tests/api/routes/guilds.test.js` * `tests/api/routes/webhooks.test.js` * `tests/api/utils/validateWebhookUrl.test.js` * `tests/api/utils/webhook.test.js` * `tests/commands/tempban.test.js` * `tests/config-listeners.test.js` * `tests/modules/cli-process.test.js` * `tests/modules/config-guild.test.js` * `tests/utils/modAction.test.js` These file types are not supported: * `.env.example` * `AGENTS.md` * `biome.json` * `src/prompts/triage-respond-system.md` * `web/package.json` * fix(prompts): replace ambiguous 'classified' with 'triaged' (Thread #11) * fix(prompts): define concrete 'flagging' mechanism for moderation (Thread #12) * fix(prompts): add PII/credentials constraint to prevent echoing secrets (Thread #13) * fix: remove bracketed-IPv6 dead code in webhook URL validator URL.hostname already strips brackets from IPv6 addresses (new URL('http://[::1]').hostname === '::1'), so the hostname.startsWith('[') branch was unreachable dead code. Remove the bracketed-IPv6 branch and the '[::1]' entry from BLOCKED_IPV6 since it could never match. Addresses CodeRabbit review thread #9. * fix: sanitize webhook URL in warning logs to prevent credential exposure Strip query string and fragment from webhook URLs before including them in warning log messages. If a webhook URL contains tokens or API keys as query parameters, they would previously appear in logs. Now logs only origin + pathname (e.g. 'https://example.com/hook') instead of the full URL with sensitive query params. Addresses CodeRabbit review thread #10. * test(config): rename misleading 'Infinity' test name (Thread #14) * fix: normalize validateSingleValue to always return string[] Unknown config paths previously returned [{path, message}] objects while validateValue returned string[]. Callers now receive a consistent string[] in both cases, eliminating the need to handle two different shapes. * test(config): move PUT partial write handling inside config routes suite (Thread #15) * refactor: extract getGuildChannels helper to remove duplication GET /:id and GET /:id/channels shared identical channel-fetching loops with a duplicated MAX_CHANNELS constant. Extracted into a single getGuildChannels(guild) helper used by both handlers. * feat: include section (topLevelKey) in PATCH config webhook payload Downstream consumers of the DASHBOARD_WEBHOOK_URL can now use the 'section' field to selectively reload only the affected config section rather than refreshing the entire config. * fix: return promise in webhook config-update handler (async/await) The previous .then()/.catch() chain was not returned, so Express 5 could not auto-forward rejected promises to the error handler. Converted to async/await so errors propagate correctly. * feat: make JSON body size limit configurable via API_BODY_LIMIT env var Defaults to '100kb' when the env var is not set, preserving existing behaviour. Operators can now tune the limit without code changes. * test(validateWebhookUrl): strengthen cache eviction test to verify re-evaluation (Thread #16) * test(webhook): move vi.useRealTimers() to afterEach to prevent timer leak (Thread #17) * refactor: move CONFIG_SCHEMA/validateValue/validateSingleValue to utils/configValidation.js validateConfigPatch.js (utils) was importing validateSingleValue from routes/config.js — an inverted dependency. Created src/api/utils/configValidation.js as the canonical home for CONFIG_SCHEMA, validateValue, and validateSingleValue. - config.js now imports from ../utils/configValidation.js and re-exports validateSingleValue for backward compatibility with existing callers. - validateConfigPatch.js now imports from ./configValidation.js directly. * perf: split path string once in validateConfigPatchBody path.split('.') was called twice — once to extract topLevelKey and again for segments. Moved the single split to the top and derived topLevelKey from segments[0], avoiding the redundant allocation. * fix(#18): change lint script from tsc to biome check * fix(#19,#20): simplify params type; add PATCH body value check * fix(#21): add metadata export to config page * fix(#22): compute addedCount/removedCount inside useMemo loop * fix(#23,#24,#25,#26): tighten isGuildConfig; extract inputClasses; guard number inputs; rename DiscardChangesButton * fix(#27): change aria-live from polite to off on char counter * fix(#28): change Toaster theme from dark to system * fix(#29,#30): export BotApiConfig; return 504 on AbortError/TimeoutError * fix(#31): add one-time localStorage key migration from old key * fix(#32,#33,#34): SpamConfig JSDoc; collapse WritableConfigSection; fix SYSTEM_PROMPT_MAX_LENGTH JSDoc * fix: remove unused validateSingleValue import, fix biome formatting in config.js After the refactor, validateSingleValue is re-exported directly via 'export { } from' and no longer needs a local import binding. Also removed an extra blank line that biome flagged as a format error. * fix: add DNS resolution validation to prevent SSRF via DNS rebinding Add async validateDnsResolution() that resolves a webhook hostname via DNS and checks all resolved IPs against the existing blocked ranges before fetch. This closes the TOCTOU gap where a hostname could pass string-based validation then resolve to a private IP at request time (DNS rebinding attack). Changes: - Add validateDnsResolution() with resolve4/resolve6 checks - Integrate DNS check in fireAndForgetWebhook before fetch - Normalize IPv6 hostnames by stripping brackets (Node.js v22 retains them in URL.hostname, contrary to WHATWG spec) - Add comprehensive test coverage for DNS rebinding scenarios - Update webhook tests for async DNS validation flow Addresses CodeRabbit review thread #8. * fix: update test assertions for string[] return type from validateSingleValue * fix: mock validateDnsResolution in webhook integration tests After adding DNS resolution pinning in fireAndForgetWebhook, the config and guilds route tests need to mock validateDnsResolution to return true so fetch is actually called. * fix: address minor code review feedback - JSDoc, tests, caps * fix(frontend): address code review feedback - HTML, types, perf * fix(backend): address code review feedback - validation, logging, exports * fix: correct IPv6 validation for public addresses and literals * fix: restore classifier invocation in triage module * fix: address test failures in validateConfigPatch and triage-respond - Check for empty path segments before topLevelKey validation - Fix test to use valid nullable path (welcome.channelId) - Add mock cleanup between triage-respond tests * fix(validation): handle alertChannelId nullable and DNS edge cases * fix(security): prevent mask sentinel write-back and auth secret override 1. configAllowlist: Add isMasked() and stripMaskedWrites() to detect and filter out writes where sensitive fields contain the mask sentinel ('••••••••'). Prevents clients from accidentally overwriting real secrets with the placeholder returned by maskSensitiveFields(). 2. bot-api-proxy: Reorder header spread so x-api-secret is always set AFTER spreading options.headers, preventing caller-provided headers from overriding the server-side auth secret. Both fixes include comprehensive tests. * test: add missing test cases for mask sentinel, prototype pollution, DNS edge cases * refactor: simplify webhook validation for internal-only use * refactor: remove unused SSRF validation code Deleted validateWebhookUrl.js and its tests since webhooks are internal-only. Simplified webhook.js to just check URL format. * fix: prevent WebSearch notification failures from aborting response * fix: correct safeSend mock setup in triage-respond tests * fix(security): use own-property checks in config validation * fix: export MASK constant and clean up orphaned JSDoc * fix: report written sections in webhook, add roles endpoint test * fix: address remaining PR review feedback - Add nullable: true to triage.moderationLogChannel and debugFooterLevel - Add evalClient param to runResponder JSDoc - Convert SAFE_CONFIG_KEYS to Set for O(1) lookups - Reorder validation checks for consistent 400 responses - Update tests for Set-based SAFE_CONFIG_KEYS --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: AnExiledDev <AnExiledDev@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Move AI conversation history from in-memory
Mapto PostgreSQL with write-through caching. Conversations now survive restarts, support configurable history depth, and auto-clean old messages.Changes
DB Schema (
src/db.js)conversationstable:id,channel_id,role,content,username,created_at(channel_id, created_at)for efficient lookupsinitDb()alongside existing config tableAI Module (
src/modules/ai.js) — +329 linesaddToHistory()writes to both in-memory cache and DB simultaneouslygetHistory()returns from cache; falls back to DB on cache missai.historyTTLDays(default 30)DATABASE_URLis not set — no crashesConfig (
config.json)ai.historyLength— max messages per channel (default:20)ai.historyTTLDays— auto-delete messages older than N days (default:30)/config set ai.historyLength 50etc.Startup/Shutdown (
src/index.js)Tests
tests/db.test.jsandtests/modules/ai.test.jsFiles Changed