feat: add gap logging functionality and improve bot thread management#6
Open
kpj2006 wants to merge 4 commits into
Open
feat: add gap logging functionality and improve bot thread management#6kpj2006 wants to merge 4 commits into
kpj2006 wants to merge 4 commits into
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 40 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe bot now persists failure logs, retries Ollama requests up to 3 times, builds conversation context from recent thread history, creates dedicated Discord threads for responses with auto-archival, serializes Ollama calls with a lock, and surfaces gap reasons when operations fail. ChangesEnhanced Discord Bot with Resilience and Thread-based Conversation
Sequence Diagram(s)sequenceDiagram
participant User
participant Discord
participant bot
participant Ollama
participant gap_log
User->>Discord: post message in main channel
Discord->>bot: on_message event
bot->>bot: determine thread context
bot->>Discord: create or reuse thread
Discord-->>bot: thread
bot->>bot: build conversation context from thread history
bot->>bot: acquire ollama_lock
loop Retry up to MAX_RETRIES
bot->>Ollama: POST prompt with context
Ollama-->>bot: response or timeout
end
bot->>bot: truncate if long reply
bot->>Discord: send/reply in thread
Discord-->>User: response in thread
alt gap occurred
bot->>gap_log: _log_gap(query, reason, thread_id)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
github-actions
Bot
added
no-issue-linked
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Line 331: Update the .gitignore to include gap_log.json so runtime bot logs
are not committed (add a line "gap_log.json"); if gap_log.json is already
tracked remove it from the index with git rm --cached gap_log.json and commit
the .gitignore change; reference .gitignore and the filename gap_log.json when
making the change to ensure future runtime artifacts are excluded.
In `@bot.py`:
- Around line 214-219: The current code treats both Ollama fallback and missing
skill context as operational "gaps" via _log_gap, which is misleading when
Ollama responded successfully but no skill file exists; change the logging so
only used_fallback triggers _log_gap(message.content, "ollama_unavailable",
thread_id=thread.id), and when skill_context is missing but a response was
produced call a configuration/validation logger (e.g., _log_warning or a new
_log_config_warning) with a distinct tag like "missing_skill_context" (still
include message.content and thread.id) instead of logging it as a gap; update
the conditional around used_fallback and skill_context to implement this
distinction and reference the symbols used_fallback, skill_context, _log_gap,
message.content, and thread.id.
- Around line 169-173: The current guard lets any Thread through because
is_in_thread bypasses the channel ID check; update the condition so when
message.channel is a discord.Thread you compare its parent channel ID (use
message.channel.parent_id or message.channel.parent.id) against
DISCORD_CHANNEL_ID_INT instead of the thread's own id; specifically change the
is_in_configured_channel logic to detect Thread vs TextChannel and, for Threads,
check parent_id == DISCORD_CHANNEL_ID_INT, and only allow when either a
non-thread channel id matches or a thread's parent matches.
- Around line 137-161: The _get_or_create_thread function currently tries to
call message.create_thread even when the message is already inside an
archived/locked discord.Thread; to fix, detect if isinstance(message.channel,
discord.Thread) and if thread.archived or thread.locked then immediately return
None (instead of falling through), so you don't attempt to create a nested
thread via message.create_thread; update the early branch that checks
thread.archived and thread.locked to return None and log a warning, leaving the
rest of the try/except creating thread logic unchanged.
- Around line 54-65: The _log_gap function does an unsynchronized
read-modify-write using _load_gap_log and _save_gap_log causing race conditions;
convert _log_gap into an async function that uses a module-level asyncio.Lock
(e.g., _gap_log_lock) to guard the load/append/save sequence, or alternatively
implement atomic file writes (write to a temp file then os.replace) inside the
critical section; then update all call sites (the async callers mentioned at
lines around where it's invoked) to await _log_gap so the lock is respected and
concurrent invocations won't clobber gap_log.json.
- Around line 115-134: The _build_conversation_context function can duplicate
the user's current message because thread.history(...) may include it; modify
_build_conversation_context to skip the current message when building
history—e.g., inside the async for loop over thread.history in
_build_conversation_context, compare each msg to the current message (use
current_author and current_query or preferably msg.id if the caller can pass the
current message id) and continue (skip) when they match so you don't include the
same content twice before you append the "Current question..." line.
- Around line 187-192: The bare except in the message.reply fallback should not
silently swallow errors; update the try/except around the message.reply call to
catch Exception as e and log the exception (e.g., using an existing logger or
logger.exception) with context like "Failed to send thread-creation failure
reply" so you retain the fallback behavior but capture the stack/exception for
debugging; keep the reply attempt logic intact and do not re-raise.
- Around line 196-201: The typing indicator currently enters and immediately
exits because async with thread.typing(): contains only pass; move the Ollama
response generation call(s) (the coroutine that generates the reply) inside the
async with thread.typing() block so the context is held for the duration of
generation; update the try/except around thread.typing() to cover the await that
performs generation (refer to thread.typing and the coroutine/function used to
call Ollama response generation) so logger.warning still captures exceptions
from both typing and generation.
In `@gap_log.json`:
- Around line 1-32: Remove the runtime log file gap_log.json from version
control and prevent future commits: run git rm gap_log.json to remove it from
the repo and add gap_log.json to .gitignore so the file created by the bot's
_save_gap_log() in bot.py is ignored; ensure you commit the .gitignore update
and the removal together and verify no other runtime artifacts from
_save_gap_log() are tracked.
In `@start_bot_hidden.vbs`:
- Around line 3-4: Replace the hard-coded botDir with a dynamic path derived
from the script's location: use WScript.ScriptFullName and the
Scripting.FileSystemObject's GetParentFolderName to compute the directory
containing the VBS file (then append "\SkillBot" if the bot folder is a child),
and keep the existing WshShell.Run call (WshShell.Run "cmd /c cd /d """ & botDir
& """ && venv\Scripts\python.exe bot.py", 0, False) unchanged except to
reference the new botDir variable; update any variable named botDir and ensure
you create the FileSystemObject (CreateObject("Scripting.FileSystemObject")) and
use GetParentFolderName(WScript.ScriptFullName) to make the script portable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 423e24a9-7eb8-49b7-bcef-c38fa906f2ef
📒 Files selected for processing (4)
.gitignorebot.pygap_log.jsonstart_bot_hidden.vbs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| @@ -0,0 +1,32 @@ | |||
| [ | |||
Contributor
There was a problem hiding this comment.
Do we need this log in the repo? Should we perhaps gitignore it?
Member
Author
There was a problem hiding this comment.
this is intentionally left for skill-updater, will make it more polished in my future pr.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
Fixes #(issue number)
Screenshots/Recordings:
Additional Notes:
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
New Features
Bug Fixes
Chores