feat: add chime-in — bot organically joins conversations#12
feat: add chime-in — bot organically joins conversations#12BillChirico merged 8 commits intomainfrom
Conversation
New module src/modules/chimeIn.js: - Per-channel message ring buffer (capped at maxBufferSize, default 30) - After evaluateEvery messages (default 10), asks a cheap LLM (Haiku) whether the bot has something genuinely valuable to add - If YES: generates a full response via generateResponse() and sends it as a plain channel message (not a reply), then clears the buffer - If NO: resets the counter but keeps the buffer for context continuity - Concurrent evaluation guard prevents duplicate triggers - resetCounter() exported so mention handler avoids double-responding Integration in events.js: - accumulate() called after spam check, before mention check - resetCounter() called when bot is @mentioned Config (config.json chimeIn section): - enabled, evaluateEvery, model, maxBufferSize, channels, excludeChannels
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a top-level Changes
Sequence DiagramsequenceDiagram
participant Message as Message Event
participant Events as events.js
participant ChimeIn as chimeIn Module
participant EvalLLM as Evaluation LLM
participant AI as AI Response Engine
participant Channel as Discord Channel
Message->>Events: messageCreate event
Events->>ChimeIn: accumulate(message, config)
ChimeIn->>ChimeIn: append to per-channel buffer
ChimeIn->>ChimeIn: check evaluateEvery counter
alt Evaluation triggered
ChimeIn->>EvalLLM: send lightweight evaluation request
EvalLLM-->>ChimeIn: YES / NO
alt YES
ChimeIn->>AI: request contextual response
AI-->>ChimeIn: response content
ChimeIn->>Channel: post autonomous message
ChimeIn->>ChimeIn: reset counter
else NO
ChimeIn->>ChimeIn: reset counter (keep buffer)
end
end
Events->>ChimeIn: resetCounter(channelId) Note: on direct bot reply to avoid duplicates
Estimated code review effort🎯 4 (High) | ⏱️ ~60 minutes 🚥 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/events.js (1)
69-83:⚠️ Potential issue | 🟠 MajorRace condition:
accumulatecan trigger a chime-in and the mention handler fires simultaneously.
accumulate()is invoked fire-and-forget on Line 70. If the counter has already reachedevaluateEvery, the async evaluation starts immediately. Then on Line 83,resetCounter()sets the counter to 0, but the in-flight evaluation is already past the counter check and will proceed to generate and send a chime-in response. Meanwhile, the mention handler below also generates a response — resulting in two bot messages for the same trigger message.Consider one of:
awaitthe accumulate call before entering the mention branch, so evaluation completes (and possibly chimes in) before the mention path runs — then skip the mention response if a chime-in just fired.- Move
accumulate()after the mention check, only calling it when the message is not a mention/reply. This is the simplest fix and aligns with the stated goal of avoiding double responses.Option 2: only accumulate when the message is not a mention
// AI chat - respond when mentioned if (config.ai?.enabled) { const isMentioned = message.mentions.has(client.user); const isReply = message.reference && message.mentions.repliedUser?.id === client.user.id; // Check if in allowed channel (if configured) const allowedChannels = config.ai?.channels || []; const isAllowedChannel = allowedChannels.length === 0 || allowedChannels.includes(message.channel.id); if ((isMentioned || isReply) && isAllowedChannel) { // Reset chime-in counter so we don't double-respond resetCounter(message.channel.id); // ... existing mention handling ... + } else { + // Chime-in: accumulate message for organic participation (fire-and-forget) + accumulate(message, client, config, healthMonitor).catch((err) => { + console.error('ChimeIn accumulate error:', err); + }); } + } else { + // AI disabled — still accumulate for chime-in if configured independently + accumulate(message, client, config, healthMonitor).catch((err) => { + console.error('ChimeIn accumulate error:', err); + }); }
🤖 Fix all issues with AI agents
In `@config.json`:
- Around line 9-16: The chimeIn config is too aggressive: change the default
"evaluateEvery" from 1 to 10 and make "enabled" false by default unless
explicitly turned on or a non-empty "channels" list is provided; update the
"chimeIn" block so evaluateEvery: 10 and enabled: false (or add validation that
prevents enabled:true when "channels" is empty and evaluateEvery is below a safe
threshold), referencing the "chimeIn", "evaluateEvery", "enabled", and
"channels" keys to locate the setting.
In `@src/modules/chimeIn.js`:
- Around line 56-74: The current shouldChimeIn flow forwards raw
conversationText (from buffer.messages) before the system instruction, which
makes the YES/NO decision vulnerable to prompt injection; modify the messages
construction so the user conversation is clearly separated (e.g., wrap
conversationText with a structured delimiter/fence like "-----BEGIN
CONVERSATION-----" / "-----END CONVERSATION-----") and place the system
directive (the object with role: 'system' and the YES/NO instruction used by
shouldChimeIn) after that delimited user content so the evaluation model sees
the system policy last and cannot be overridden by user-supplied content.
- Around line 15-20: channelBuffers currently grows without bounds because
entries are never evicted; update the per-channel state stored in channelBuffers
to include a lastActivity timestamp (e.g., store {messages, counter,
lastActivity}) and implement an eviction policy: either enforce a max size LRU
by moving touched keys to the Map's end and trimming oldest entries when adding
in the function that updates channelBuffers, or add a periodic cleanup task
(setInterval) that removes channels whose lastActivity is older than N minutes;
ensure related logic that checks/uses channelBuffers and evaluatingChannels
(same names) gracefully handles removed entries (e.g., clear evaluatingChannels
for pruned channels).
- Line 117: The accumulate function currently declares an unused parameter
client; remove client from the accumulate function signature (export async
function accumulate(message, config, healthMonitor)) and update all call sites
(notably the call in events.js that currently passes client) to stop passing
that extra argument, and update any related JSDoc/comments to reflect the new
parameter list; run tests/lint to ensure no other references remain.
- Around line 160-168: The generateResponse call in chimeIn.js can hang and
leave the channel locked in evaluatingChannels; wrap the generateResponse
promise with a timeout (e.g., Promise.race against a timeout-rejecting Promise)
or ensure generateResponse itself accepts/implements a timeout option, then
handle the timeout by removing channelId from evaluatingChannels and
returning/logging a timeout error; specifically update the call that passes
channelId, contextMessage, '_chime-in_', config, healthMonitor so it either uses
a timed wrapper or a new timeout param to generateResponse and guarantees
cleanup of evaluatingChannels on timeout.
- Around line 127-136: The ring buffer is storing messages with empty string
content (attachments/embeds only), wasting slots; before pushing into
buf.messages in the push block, guard by checking message.content.trim() is
non-empty (i.e., only push if message.content && message.content.trim() !== ''),
so attachment-only or embed-only messages are skipped; keep the existing
trimming loop (while buf.messages.length > maxBufferSize { buf.messages.shift()
}) unchanged.
- Around line 77-103: The fetch to OPENCLAW_URL (using OPENCLAW_TOKEN, model,
messages) needs a request timeout to avoid hanging; wrap the fetch in an
AbortController (or use AbortSignal.timeout) and pass signal to fetch, start a
setTimeout to call controller.abort() after a configurable ms, and clear that
timer on success/failure so the controller is cleaned up; ensure the catch
handles AbortError the same way and the function still returns false on timeout
to allow the evaluate/accumulate logic (e.g., evaluatingChannels) to release the
channel.
In `@src/modules/events.js`:
- Around line 69-71: The call to the async function accumulate(message, client,
config, healthMonitor) is fire-and-forget and may produce unhandled promise
rejections if it throws before its internal try block; wrap the call with a
rejection handler (e.g., accumulate(...).catch(err => { /* log error via
healthMonitor or process logger */ })) or explicitly await it where appropriate,
ensuring you log the error and include context (message/client/config) so
unexpected TypeError or other sync throws are caught; update the call site that
invokes accumulate(...) accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
config.jsonsrc/modules/chimeIn.jssrc/modules/events.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/modules/events.js (1)
src/modules/chimeIn.js (2)
accumulate(117-194)resetCounter(202-207)
🔇 Additional comments (6)
src/modules/events.js (1)
9-9: LGTM on the import.Clean import of the two public exports from the new module.
src/modules/chimeIn.js (5)
28-36: LGTM. Standard get-or-create pattern.
38-51: LGTM. Exclusion-first logic is clean, and the empty allow-list convention is consistent with the existing AI channel config.
148-193: Overall evaluation/response flow is well-structured.The concurrency guard, try/catch/finally cleanup, counter-reset-on-error, and buffer-clear-on-success semantics are all sound. The separation between "YES → clear buffer" and "NO → keep buffer, reset counter" provides good context continuity.
196-207:resetCounterdoes not cancel in-flight evaluations.This is the flip side of the race condition noted in
events.js: callingresetCounterwhile an evaluation is already in progress has no effect on that evaluation — it will still generate and send a chime-in response. If the double-response race is addressed in the caller (recommended), this function is fine as-is. Otherwise, consider also checking/clearing theevaluatingChannelsentry here and aborting the in-flight fetch via anAbortControllerstored per channel.
1-13: Clean module structure and documentation.The header comment, imports, and overall organization are well done. The separation of concerns (helpers, evaluation, public API) is clear.
✏️ 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.
- config.json: change evaluateEvery from 1 to 10, set enabled to false - chimeIn.js: add LRU eviction with MAX_TRACKED_CHANNELS and inactive cleanup - chimeIn.js: mitigate prompt injection by wrapping conversation in delimiters, system instruction last - chimeIn.js: add AbortSignal.timeout(10_000) to evaluation fetch call - chimeIn.js: remove unused client parameter from accumulate() - chimeIn.js: add guard to skip empty/attachment-only messages - chimeIn.js: add Promise.race timeout to generateChimeInResponse - events.js: add .catch() to fire-and-forget accumulate() call - events.js: move accumulate after mention check to prevent double responses - chimeIn.js: use separate generateChimeInResponse to avoid polluting shared AI history - chimeIn.js: suppress error-like or empty responses from being sent as unsolicited messages - chimeIn.js: import OPENCLAW_URL/OPENCLAW_TOKEN from ai.js instead of duplicating
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/modules/chimeIn.js`:
- Around line 154-173: The code creates a redundant manual timeout
(timeoutPromise + Promise.race) around fetchPromise which already uses
AbortSignal.timeout(30_000), risking unhandled rejections if the manual timeout
wins; remove the timeoutPromise and the Promise.race call and instead await
fetchPromise directly (keep the AbortSignal.timeout on the fetch so the request
is aborted correctly), delete the setTimeout that constructs timeoutPromise, and
ensure any fetch rejections from fetchPromise are caught/handled where response
is used.
- Line 193: The accumulate function currently accepts a healthMonitor parameter
but never uses it; either remove healthMonitor from accumulate's signature and
all call sites in events.js, or integrate it into the API workflow by
instrumenting the calls that generateChimeInResponse makes (mirroring
generateResponse in ai.js): call healthMonitor.recordAIRequest() before the
external request, wrap the HTTP call to OPENCLAW_URL and call
healthMonitor.setAPIStatus(success|failure) based on the response or caught
error, and ensure healthMonitor is forwarded into generateChimeInResponse so the
monitoring methods are invoked there.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
config.jsonsrc/modules/ai.jssrc/modules/chimeIn.jssrc/modules/events.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/modules/chimeIn.js (2)
src/modules/ai.js (6)
messages(78-82)response(88-99)OPENCLAW_URL(29-29)OPENCLAW_URL(29-29)OPENCLAW_TOKEN(30-30)OPENCLAW_TOKEN(30-30)src/logger.js (2)
warn(225-227)info(218-220)
src/modules/events.js (1)
src/modules/chimeIn.js (2)
resetCounter(277-282)accumulate(193-269)
⏰ 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 (9)
src/modules/ai.js (1)
28-30: LGTM — exporting shared API constants avoids duplication.Clean approach to share the endpoint/token with the new chimeIn module rather than duplicating the env-var reads.
config.json (1)
9-16: Safe defaults — previous review concerns addressed.
enabled: falseandevaluateEvery: 10are sensible production-safe defaults.src/modules/events.js (2)
69-80: Correct placement ofresetCounter— prevents double responses.Resetting the chime-in counter immediately upon detecting a mention, before generating the AI response, correctly avoids the race where a chime-in evaluation could fire while the mention response is in flight.
111-119: Clean integration — early return and.catch()are both correct.The
returnon line 112 correctly prevents mentioned messages from also being accumulated into the chime-in buffer. The.catch()onaccumulate()addresses the previously flagged fire-and-forget concern.src/modules/chimeIn.js (5)
1-24: Well-structured module header and state declarations.Clean separation of concerns with per-channel state, concurrency guard, and configurable eviction constants. The 100-channel LRU cap and 30-minute inactivity timeout are reasonable defaults for a Discord bot.
31-48: Eviction logic is sound.Inactive-channel pruning followed by LRU overflow eviction is a clean two-pass approach. Map deletion during
for...ofiteration is safe per the JS spec.
53-76: LGTM —getBufferandisChannelEligibleare clean.Eviction is triggered only on new-channel insertion (line 55), avoiding unnecessary overhead. Channel eligibility logic correctly gives
excludeChannelspriority over the allow-list.
81-131: Evaluation function is well-guarded.10s timeout,
max_tokens: 10, safefalsedefault on error, andstartsWith('YES')parsing are all solid choices for a cheap gate-keeping call.
277-292: LGTM — simple, correct public API helpers.
resetCounterandisEvaluatingare minimal and correct.
✏️ 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: 2
🤖 Fix all issues with AI agents
In `@src/modules/chimeIn.js`:
- Around line 232-245: The current startsWith-based suppression
(response.startsWith('Sorry, I') / response.startsWith('Error')) is brittle;
update the logic in the block that decides whether to send messages (the code
using response, warn, and message.channel.send) to only suppress truly empty or
whitespace-only responses and/or rely on an explicit error signal from the
upstream API (e.g., a response.error flag or status code) instead of
string-prefix matching; remove or make the string heuristics configurable (e.g.,
a blacklist array or option) and ensure chunking via response.match and sending
via message.channel.send remains unchanged for valid responses.
- Around line 137-174: The generateChimeInResponse function currently injects
raw conversationText into the user prompt; update generateChimeInResponse to
wrap conversationText with the same structured delimiter used in shouldChimeIn
(e.g., surround the conversation with "<conversation>" and "</conversation>")
before building the messages array so the user message uses the delimited block,
keeping the rest of the systemPrompt/messages construction, model, and fetch
logic unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/modules/chimeIn.jssrc/modules/events.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/modules/chimeIn.js (2)
src/modules/ai.js (9)
systemPrompt(72-75)messages(78-82)response(88-99)OPENCLAW_URL(29-29)OPENCLAW_URL(29-29)OPENCLAW_TOKEN(30-30)OPENCLAW_TOKEN(30-30)data(108-108)reply(109-109)src/logger.js (2)
warn(225-227)info(218-220)
src/modules/events.js (1)
src/modules/chimeIn.js (2)
resetCounter(269-274)accumulate(185-261)
⏰ 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 (6)
src/modules/events.js (1)
9-9: Chime-in integration looks clean.The flow is well-structured: counter reset before mention processing prevents double-responses, the guard
returnon Line 112 correctly excludes direct mentions from the chime-in buffer, and the fire-and-forget.catch()addresses the previously flagged unhandled rejection concern. Theaccumulatecall signature now correctly matches the updated(message, config)parameters.Also applies to: 79-80, 111-112, 116-119
src/modules/chimeIn.js (5)
1-24: Module structure and state management look solid.Clean separation of concerns with well-documented module header. The LRU eviction constants (
MAX_TRACKED_CHANNELS,CHANNEL_INACTIVE_MS) address the previously flagged unbounded memory growth concern.
31-61: Eviction and buffer management are correct.
evictInactiveChannelsproperly handles both time-based and size-based eviction. Deleting from aMapduringfor...ofiteration is safe per the JS spec.getBuffertriggers eviction only on new channel creation, avoiding unnecessary work on every access.
185-261: Concurrency guard and error handling are well-structured.The
evaluatingChannelsSet +try/finallypattern correctly prevents concurrent evaluations and guarantees cleanup. Counter reset on error (Line 257) prevents spin-looping. The early-return cascade (disabled → ineligible → empty → below threshold → already evaluating) is clean and efficient.One minor note: messages arriving during an ongoing evaluation still push into
buf.messagesand incrementbuf.counter(lines 199–210 execute before the guard at line 216). When the evaluation completes and resets the counter, those interim increments are lost. This is acceptable behavior but worth being aware of — under heavy traffic during a slow evaluation, the effective evaluation cadence will be longer thanevaluateEvery.
269-274: LGTM.Simple and correct. No-op when the channel has no buffer is the right behavior.
92-101: This message ordering is already intentional—review the comment on line 91.The code deliberately places
userbeforesystemwith an explicit comment explaining it's for prompt injection mitigation. This differs from the standard ordering inai.js(line 78–82), wheresystemcomes first. Since OpenClaw is OpenAI-compatible and the code is already deployed, the backend evidently respects trailingsystemmessages. Consider documenting this design choice or adding a comment referencing OpenClaw's message-ordering behavior if clarity is needed for future maintainers.Likely an incorrect or invalid review comment.
✏️ 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.
- Wrap conversation text in <conversation> delimiters in generateChimeInResponse to match shouldChimeIn pattern (fixes inconsistent prompt structure) - Replace brittle string-prefix error detection (startsWith 'Sorry, I' / 'Error') with simple empty/whitespace check to avoid false positives and negatives - Add AbortController cancellation mechanism to prevent double-responses when bot is @mentioned during an in-flight chime-in evaluation
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/modules/chimeIn.js`:
- Around line 15-17: The JSDoc type for channelBuffers is missing the
abortController field; update the inline type annotation for the Map (the
comment on the Map<channelId, { ... }>) to include abortController:
AbortController (or nullable) alongside messages, counter, and lastActive so it
matches the actual stored object used elsewhere (see channelBuffers and the
abortController assigned around where fetch/stream is started). Ensure the
symbol names remain messages, counter, lastActive, and abortController to keep
the annotation accurate.
- Around line 253-257: The current chunking in chimeIn.js uses /[\s\S]{1,1990}/g
which can break mid-word/URL/emoji; update the logic that builds chunks from the
response (the variable named "response" and the "chunks" array used before
calling message.channel.send) to split on natural boundaries—preferably break on
newline or whitespace boundaries (e.g., accumulate words/lines until adding the
next would exceed 1990 chars, then start a new chunk) so each chunk ends at a
space or newline; ensure you keep the 1990 char limit and that the for-loop
sending via message.channel.send remains unchanged.
- Around line 219-246: The AbortController created as abortController is only
checked between steps and its signal is never passed into the HTTP-requesting
functions, so shouldChimeIn and generateChimeInResponse continue running even
after resetCounter aborts; update shouldChimeIn(...) and
generateChimeInResponse(...) call sites to accept an AbortSignal (e.g., pass
abortController.signal) and ensure the internal fetch/http calls use that signal
(and merge it with any timeout logic using AbortSignal.any on Node ≥18.17.0 or
by creating a per-request AbortController that you abort from
abortController.signal.onabort for older Node), keeping the existing
abortController.signal checks as fallback and ensuring request-level
cancellation actually aborts network I/O.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/modules/chimeIn.js
⏰ 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 (6)
src/modules/chimeIn.js (6)
1-14: Clean module header and imports.Well-structured module documentation that clearly explains the feature's behavior. Imports are minimal and relevant.
31-48: Eviction logic is sound.Inactivity-based eviction followed by LRU-style overflow eviction is a clean approach. Map deletion during
for...ofiteration is safe in JavaScript.
63-76: LGTM — clear inclusion/exclusion logic.
81-131: Evaluation function looks solid.Good use of
AbortSignal.timeout, structured<conversation>delimiters, system-instruction-last ordering, and safe fallback tofalseon error.
137-174: Response generation function is well-structured.Separate AI context avoids polluting shared history. Timeouts and
<conversation>delimiters are in place.
279-296:resetCountercleanly cancels in-flight evaluations.Properly resets the counter and aborts any pending evaluation to prevent double-responses on
@mention.
✏️ 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.
- Update JSDoc type annotation to include abortController field - Pass AbortSignal to shouldChimeIn/generateChimeInResponse fetch calls - Replace regex chunking with whitespace-aware word-boundary splitting - Reorder system message before user message in shouldChimeIn
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/modules/chimeIn.js`:
- Around line 261-274: The fallback hard-cut at splitAt = 1990 can split UTF-16
surrogate pairs (e.g., emoji); modify the fallback logic around splitAt (used
with remaining.lastIndexOf and remaining.slice) to ensure you never cut in the
middle of a surrogate pair: when splitAt <= 0 and you set splitAt = 1990, check
surrounding code units via remaining.charCodeAt(splitAt - 1) /
charCodeAt(splitAt) and, if the cut would split a surrogate pair (high surrogate
at split boundary), decrement splitAt until the boundary is safe (or > 0) before
slicing and pushing the chunk.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/modules/chimeIn.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/modules/chimeIn.js (2)
src/modules/ai.js (7)
systemPrompt(72-75)messages(78-82)response(88-99)OPENCLAW_URL(29-29)OPENCLAW_URL(29-29)OPENCLAW_TOKEN(30-30)OPENCLAW_TOKEN(30-30)src/logger.js (2)
warn(225-227)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 (6)
src/modules/chimeIn.js (6)
1-14: Clean module documentation and imports.The module header clearly explains the feature's behavior, and imports are minimal and correctly sourced.
31-48: Eviction logic looks solid.Two-phase eviction (time-based, then LRU overflow) is a clean approach. One subtlety: evicted entries with a non-null
abortControllerwon't have their in-flight fetches aborted. In practice this is a non-issue becausegetBufferrefresheslastActiveon every access, so channels mid-evaluation won't be inactive. Worth a brief note if this assumption ever changes.
81-135: Evaluation function is well-structured.Signal composition via
AbortSignal.any, structured<conversation>delimiters, system-first message ordering, and safe fallback tofalseon any error — all previous feedback has been addressed cleanly.
162-174: Thrown errors fromgenerateChimeInResponsepropagate correctly to the caller's catch block.The asymmetry with
shouldChimeIn(which swallows errors and returnsfalse) is intentional and correct:accumulate's try/catch handles the thrown error, resets the counter, and cleans upevaluatingChannelsinfinally.
193-297:accumulatecontrol flow is well-orchestrated.The concurrency guard, abort-signal threading, post-evaluation cancellation checks, and cleanup in
finallyare all correct. Previous feedback (unused params, redundantPromise.race, brittle error checks, empty-message guard) has been addressed. The counter-reset semantics on both YES/NO/error paths are consistent.
305-316:resetCountercorrectly cancels in-flight evaluations.Aborting the controller and nulling it prevents double-responses when the bot is
@mentionedduring an active chime-in evaluation cycle.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: |
- Create src/utils/splitMessage.js with word-boundary-aware splitting logic - Replace inline splitting in chimeIn.js with shared utility - Replace simple regex splitting in events.js with shared utility - Both modules now use consistent message-splitting behavior Fixes duplicated message-splitting logic that had divergent behavior: - chimeIn.js used word-boundary-aware splitting - events.js used simple regex that could split mid-word/URL Co-authored-by: Cursor Agent <cursoragent@cursor.com>
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
Replace permanent mem0Available = false on API errors with a cooldown- based recovery mechanism. After RECOVERY_COOLDOWN_MS (60s), the next request is allowed through to check if the service recovered. If it fails again, the cooldown resets. This prevents a single transient network error from permanently disabling the memory system for all users until restart. Resolves review threads #4, #8, #12.
Replace permanent mem0Available = false on API errors with a cooldown- based recovery mechanism. After RECOVERY_COOLDOWN_MS (60s), the next request is allowed through to check if the service recovered. If it fails again, the cooldown resets. This prevents a single transient network error from permanently disabling the memory system for all users until restart. Resolves review threads #4, #8, #12.
Replace permanent mem0Available = false on API errors with a cooldown- based recovery mechanism. After RECOVERY_COOLDOWN_MS (60s), the next request is allowed through to check if the service recovered. If it fails again, the cooldown resets. This prevents a single transient network error from permanently disabling the memory system for all users until restart. Resolves review threads #4, #8, #12.
- Differentiate requireGuildAdmin (ADMINISTRATOR only) from requireGuildModerator (ADMINISTRATOR | MANAGE_GUILD), aligning REST admin check with slash-command isAdmin (#1, #2, #12) - Add botOwners startup warning when using default upstream ID (#3) - Add SESSION_SECRET, DISCORD_CLIENT_SECRET, DISCORD_REDIRECT_URI to README deployment table (#4) - Pass actual permission level to getPermissionError so modlog denial says 'moderator' not 'administrator' (#5) - Guard _seedOAuthState with NODE_ENV production check (#6) - Add test: valid JWT with no server-side session (#7) - Add DiscordApiError class with HTTP status (#8) - Add moderatorRoleId support to isModerator (#9) - Remove no-op delete override from SessionStore (#10) - Cap oauthStates at 10k entries (#11) - Fix hasOAuthGuildPermission docstring for bitwise OR semantics (#12) - Handle dashboard URL fragment collision (#13) - Cap guildCache at 10k entries (#14) - Add SESSION_TTL_MS co-location comment with JWT expiry (#15) - Cache SESSION_SECRET via lazy getter in verifyJwt (#16) - Remove PII (username) from OAuth auth log (#17)
- Differentiate requireGuildAdmin (ADMINISTRATOR only) from requireGuildModerator (ADMINISTRATOR | MANAGE_GUILD), aligning REST admin check with slash-command isAdmin (#1, #2, #12) - Add botOwners startup warning when using default upstream ID (#3) - Add SESSION_SECRET, DISCORD_CLIENT_SECRET, DISCORD_REDIRECT_URI to README deployment table (#4) - Pass actual permission level to getPermissionError so modlog denial says 'moderator' not 'administrator' (#5) - Guard _seedOAuthState with NODE_ENV production check (#6) - Add test: valid JWT with no server-side session (#7) - Add DiscordApiError class with HTTP status (#8) - Add moderatorRoleId support to isModerator (#9) - Remove no-op delete override from SessionStore (#10) - Cap oauthStates at 10k entries (#11) - Fix hasOAuthGuildPermission docstring for bitwise OR semantics (#12) - Handle dashboard URL fragment collision (#13) - Cap guildCache at 10k entries (#14) - Add SESSION_TTL_MS co-location comment with JWT expiry (#15) - Cache SESSION_SECRET via lazy getter in verifyJwt (#16) - Remove PII (username) from OAuth auth log (#17)
… (#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>

Summary
Adds a chime-in feature that lets the bot automatically participate in guild conversations without being explicitly @mentioned.
How it works
Key design decisions
maxTokens: 10(just YES/NO) for minimal costFiles changed
src/modules/chimeIn.js— New module (207 lines)src/modules/events.js— Integration hooksconfig.json— AddedchimeInconfig sectionConfig
Inspired by openclaw/openclaw#10652.