Feature/nestbot ai assistant#3699
Feature/nestbot ai assistant#3699rajnisk wants to merge 27 commits intoOWASP:feature/nestbot-ai-assistantfrom
Conversation
- Refactor QuestionDetector to use CrewAI agents with fallback prompts to prevent ObjectDoesNotExist errors. - Update Makefile to include the 'core' app in dump-data for preserving prompts in fixtures.
…ability fixes - Implement collaborative multi-agent orchestration with intent-based routing - Move Slack mentions and commands to async background processing - Fix missing prompt crashes and refactor legacy question detection - Complete benchmarking and update documentation
- Fix reaction cleanup: remove open_eye_reaction when no answer or on success - Enhance RAG agent with multi-query search strategy - Add greeting detection for simple messages like "Hello" - Update router to better classify greetings - Expand test_ai_assistant with comprehensive test queries Resolves issues with stuck reactions, unhelpful RAG responses, and incorrect greeting processing.
- Fix reaction cleanup: remove open_eye_reaction when no answer or on success - Enhance RAG agent with multi-query search strategy - Add greeting detection for simple messages like "Hello" - Update router to better classify greetings - Expand test_ai_assistant with comprehensive test queries Resolves issues with stuck reactions, unhelpful RAG responses, and incorrect greeting processing.
|
Important Review skippedToo many files! This PR contains 297 files, which is 147 over the limit of 150. You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughSwitches LLM/embedder config to Django settings with Google support; adds GoogleEmbedder; exposes optional Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
Pull request overview
This PR implements fixes for NestBot AI Assistant addressing reaction management, Question Detector output prevention, Slack block character limit handling, and adds Google Gemini support for local development.
Changes:
- Enhanced reaction management with "thinking" messages and automatic cleanup of reactions
- Added multi-level validation to prevent Question Detector "YES"/"NO" responses from being posted
- Implemented automatic splitting of long responses exceeding Slack's 3001 character block limit
- Added Google Gemini as an alternative LLM provider with corresponding embedding support
- Introduced greeting detection to handle simple greetings before agent routing
- Modified AI command handling to use async processing
- Added collaborative multi-agent flow for low-confidence queries
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 28 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/settings/base.py | Added Gemini configuration settings and changed OpenAI secret key handling |
| backend/apps/slack/services/message_auto_reply.py | Added reaction management, thinking messages, and new async processing function |
| backend/apps/slack/events/app_mention.py | Changed to async processing with reaction management |
| backend/apps/slack/events/message_posted.py | Added bot mention detection logic |
| backend/apps/slack/common/question_detector.py | Switched from direct OpenAI calls to CrewAI agent-based detection |
| backend/apps/slack/common/handlers/ai.py | Added format_blocks function for handling Slack character limits |
| backend/apps/slack/commands/ai.py | Completely rewrote to use async processing |
| backend/apps/ai/templates/router/tasks/route.jinja | Added greeting classification guidelines |
| backend/apps/ai/templates/agents/rag/backstory.jinja | Enhanced search strategy instructions |
| backend/apps/ai/router.py | Added validation for Question Detector output and improved error handling |
| backend/apps/ai/flows/assistant.py | Added greeting handling and collaborative multi-agent flow |
| backend/apps/ai/embeddings/google.py | New Google embedding API integration |
| backend/apps/ai/embeddings/factory.py | Added Google embedder selection |
| backend/apps/ai/common/llm_config.py | Added Gemini support and changed fallback behavior |
| backend/apps/ai/agents/*/agent.py | Added allow_delegation parameter to all agent creation functions |
| backend/apps/ai/management/commands/test_ai_assistant.py | New management command for testing AI assistant |
| backend/Makefile | Added 'core' app to data dump command |
| .gitignore | Added .plan/ directory exclusion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@backend/apps/ai/common/llm_config.py`:
- Around line 24-30: The model name for the Google provider is incorrectly
prefixed with "openai/"; in the block that returns an LLM when provider ==
"google" (the LLM constructor call using settings.GOOGLE_MODEL_NAME), remove the
"openai/" prefix and pass settings.GOOGLE_MODEL_NAME directly so the model
argument becomes the raw model name (e.g., "gemini-2.5-flash") instead of
"openai/<model>".
In `@backend/apps/ai/embeddings/factory.py`:
- Around line 19-23: The factory can return 768-dim GoogleEmbedder or 1536-dim
OpenAIEmbedder which will clash with the hard-coded VectorField dimensions=1536
on the Chunk model; update code so embeddings are validated or the field is
configurable: add a validation in Chunk.update_data() to check incoming
embedding length against the current VectorField dimension (raise/log a clear
error if mismatch), or make the VectorField dimension configurable from
settings.LLM_PROVIDER and add a DB migration to alter the pgvector column when
provider changes; also fix the factory docstring to reference Google (or the
specific Gemini/Google model name) instead of "gemini". Ensure you refer to
settings.LLM_PROVIDER, GoogleEmbedder, OpenAIEmbedder, Chunk.update_data, and
the VectorField dimension when implementing the change.
In `@backend/apps/slack/common/handlers/ai.py`:
- Around line 47-55: The code double-processes link formatting:
format_ai_response_for_slack(text) already calls format_links_for_slack(), and
markdown(...) in blocks.py calls it again; remove the redundant call to
format_links_for_slack() from one of those places to avoid wasted work.
Recommended change: delete the format_links_for_slack() invocation inside
format_ai_response_for_slack (leave markdown(...) as the single place that
normalizes links), update function comments to note markdown() will handle link
formatting, and run any existing tests to ensure no behavioral change;
alternatively, if you prefer the inverse, remove the call from markdown() and
document that format_ai_response_for_slack performs link formatting. Ensure you
update references to format_ai_response_for_slack, markdown, and
format_links_for_slack accordingly.
In `@backend/apps/slack/common/question_detector.py`:
- Around line 115-128: The current parsing of crew.kickoff() uses substring
checks ("YES" in result / "NO" in result) which can yield false positives;
change detection to require precise matches by normalizing the result
(strip/upper as already done) and then either check equality (result == "YES" /
result == "NO"), require the result to start with the token
(result.startswith("YES") / result.startswith("NO")), or use a regex with
word-boundary anchors (e.g., re.match(r'^(YES|NO)\b', result)) to avoid matching
embedded words; keep existing logger.warning and logger.exception behavior but
ensure the matched branches only trigger on these stricter conditions.
In `@backend/apps/slack/events/message_posted.py`:
- Around line 49-75: Cache the Slack bot user id to avoid calling
client.auth_test() on every message: introduce a module-level variable (e.g.
cached_bot_user_id) and use it in the message handler instead of calling
client.auth_test() each time; call client.auth_test() only when
cached_bot_user_id is None or invalid, set cached_bot_user_id =
bot_info.get("user_id") after a successful call, and then use cached_bot_user_id
in the existing mention checks (the f"<@{bot_user_id}>" text check and the block
traversal that compares text_element.get("user_id") to bot_user_id). Ensure
error handling remains for auth_test() failures and that the rest of the logic
(bot_mentioned flag and block parsing) uses the cached value.
In `@backend/apps/slack/services/message_auto_reply.py`:
- Around line 333-342: The current logger.debug call in message_auto_reply.py
logs the full blocks payload (blocks = format_blocks(ai_response_text)), which
may contain PII and large outputs; change it to avoid logging the entire blocks
object and instead log only the blocks_count (len(blocks)) and a short,
sanitized preview (e.g., first N characters or first 1–2 blocks truncated)
and/or a stable hash/metadata; update the logger.debug invocation that
references channel_id, message_ts and blocks to include counts and a
truncated_preview or blocks_hash field rather than the full blocks variable, and
ensure any preview is sanitized to remove sensitive content before logging.
In `@backend/settings/base.py`:
- Around line 221-225: Change GOOGLE_API_KEY from values.Value to
values.SecretValue to ensure it is treated as a secret like OPEN_AI_SECRET_KEY
and other credentials; locate the GOOGLE_API_KEY declaration in the settings
module (alongside OPEN_AI_SECRET_KEY, OPENAI_MODEL_NAME, GOOGLE_MODEL_NAME,
LLM_PROVIDER) and replace the initializer so the variable uses
values.SecretValue() instead of values.Value(default=None), preserving the
default behavior for absent keys while ensuring secret handling.
🧹 Nitpick comments (14)
backend/apps/ai/agents/project/agent.py (1)
17-17: Consider makingallow_delegationkeyword-only.Ruff flags FBT001/FBT002 for boolean positional arguments. Since this pattern is used consistently across all agent factories in this PR, consider making it keyword-only to prevent accidental positional usage and improve call-site clarity.
♻️ Proposed fix
-def create_project_agent(allow_delegation: bool = False) -> Agent: +def create_project_agent(*, allow_delegation: bool = False) -> Agent:backend/apps/ai/agents/chapter/agent.py (1)
10-15: Docstring missingArgssection forallow_delegation.Unlike the other agent factories (project, contribution, rag), this function's docstring doesn't document the new
allow_delegationparameter. Consider adding the Args section for consistency.📝 Proposed fix
def create_chapter_agent(allow_delegation: bool = False) -> Agent: """Create Chapter Expert Agent. + Args: + allow_delegation (bool): Whether the agent can delegate tasks. Defaults to False. + Returns: Agent: Chapter Expert Agent configured with chapter tools """backend/apps/ai/management/commands/test_ai_assistant.py (1)
86-97: Consider extracting the truncation limit as a constant.The magic value
500could be extracted to a named constant for clarity, though this is a minor improvement for a testing utility.The broad
Exceptioncatch is appropriate here since this is a benchmark command that should capture and report all failures rather than crashing.♻️ Optional improvement
+RESPONSE_PREVIEW_LENGTH = 500 + class Command(BaseCommand): help = "Benchmark the NestBot AI Assistant with a set of test queries."- response_preview = response[:500] + "..." if len(response) > 500 else response + response_preview = ( + response[:RESPONSE_PREVIEW_LENGTH] + "..." + if len(response) > RESPONSE_PREVIEW_LENGTH + else response + )backend/apps/slack/common/handlers/ai.py (3)
52-52: Consider extracting constant to module level.
MAX_BLOCK_TEXT_LENGTHis defined inside the function. For better visibility and reusability, consider moving it to module level.♻️ Suggested refactor
+MAX_BLOCK_TEXT_LENGTH = 3000 # Slack limit is 3001, leave margin for safety + + def format_blocks(text: str) -> list[dict]: """Format AI response text into Slack blocks. ... """ formatted_response = format_ai_response_for_slack(text) - - # Slack has a limit of 3001 characters per block text - # Split into multiple blocks if needed - MAX_BLOCK_TEXT_LENGTH = 3000 # Leave some margin for safety - + if len(formatted_response) <= MAX_BLOCK_TEXT_LENGTH:
155-160: Simplify comparison usinginoperator.The condition can be more concise and readable.
♻️ Suggested refactor
- if result_upper == "YES" or result_upper == "NO": + if result_upper in {"YES", "NO"}:
105-131: Consider simplifying the triple validation loop.The code performs three sequential validation passes (lines 96-103, 105-118, 120-131) to ensure blocks stay under the limit. If the initial chunking logic is correct and
format_links_for_slackdoesn't significantly expand text, the second and third passes should rarely trigger.While defensive, this adds complexity. Consider consolidating into a single robust pass or adding a comment explaining why multiple passes are necessary (e.g., if link expansion can be significant).
backend/apps/slack/common/question_detector.py (2)
50-56: Rename variable and update log message for clarity.The variable
openai_resultand the log message "OpenAI detection failed" are misleading since the detection now uses a generic LLM provider (could be Google/Gemini).♻️ Suggested refactor
- openai_result = self.is_owasp_question_with_llm(text, context_chunks) + llm_result = self.is_owasp_question_with_llm(text, context_chunks) - if openai_result is None: + if llm_result is None: logger.warning( - "OpenAI detection failed.", + "LLM detection failed.", ) return False - return openai_result + return llm_result
22-25: Consider removing unused class constants.After the refactor to use
get_llm(), the class constantsMAX_TOKENS,TEMPERATURE, andCHAT_MODELappear to be unused since LLM configuration is now handled byget_llm().♻️ Suggested cleanup
class QuestionDetector: """Utility class for detecting OWASP-related questions.""" - MAX_TOKENS = 10 - TEMPERATURE = 0.1 - CHAT_MODEL = "gpt-4o-mini" CHUNKS_RETRIEVAL_LIMIT = 10backend/apps/ai/embeddings/factory.py (1)
12-13: Minor: Docstring inconsistency.The docstring says "OpenAI and gemini" - consider standardizing to "OpenAI and Google (Gemini)" for consistency with the rest of the codebase.
♻️ Suggested fix
- Currently returns OpenAI and gemini embedder, but can be extended to support + Currently returns OpenAI or Google (Gemini) embedder, but can be extended to supportbackend/apps/ai/router.py (3)
48-49: Move logger initialization to module level.Importing and initializing the logger inside the function is non-standard. Logger setup should be at module level for consistency and slight performance benefit (avoid repeated imports).
♻️ Suggested refactor
import contextlib +import logging from crewai import Agent, Crew, Task from apps.ai.common.intent import Intent from apps.ai.common.llm_config import get_llm from apps.ai.template_loader import env +logger = logging.getLogger(__name__) + def create_router_agent() -> Agent:Then remove lines 48-49 from inside the
route()function.
81-81: Simplify comparison usinginoperator.Same pattern as noted in
handlers/ai.py- use set membership for cleaner code.♻️ Suggested refactor
- if result_upper == "YES" or result_upper == "NO": + if result_upper in {"YES", "NO"}:
100-100: Consider extracting magic number to a named constant.The value
50for minimum result length validation would be clearer as a named constant.♻️ Suggested refactor
+MIN_VALID_ROUTING_RESULT_LENGTH = 50 + def route(query: str) -> dict: ... - if not has_intent_line and len(result_str) < 50: + if not has_intent_line and len(result_str) < MIN_VALID_ROUTING_RESULT_LENGTH:backend/apps/ai/flows/assistant.py (1)
150-152: Preserve intent order when deduping to keep task sequencing deterministic.
set()randomizes order, which can change agent/task ordering across runs. Use an order‑preserving dedupe instead.♻️ Order‑preserving dedupe
- all_intents = list(set(all_intents)) # Deduplicate + all_intents = list(dict.fromkeys(all_intents)) # Deduplicate, keep orderbackend/apps/ai/embeddings/google.py (1)
14-47: Add API key validation to prevent "Bearer None" errors.The endpoint and Bearer auth header are correct per Google's OpenAI-compatible embeddings API. However, since
GOOGLE_API_KEYdefaults toNonein settings, add an early guard in__init__to fail fast if the key is missing—matching the pattern used elsewhere in the codebase (e.g.,backend/apps/common/eleven_labs.py).🔒 Add API key validation
def __init__(self, model: str = "text-embedding-004") -> None: """Initialize Google embedder. Args: model: The Google embedding model to use. """ self.api_key = settings.GOOGLE_API_KEY + if not self.api_key: + raise ValueError("GOOGLE_API_KEY is not configured") self.model = model self.endpoint = "https://generativelanguage.googleapis.com/v1beta/openai/embeddings" self._dimensions = 768 # text-embedding-004 dimensions
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please follow our guidelines.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/apps/slack/services/message_auto_reply.py`:
- Around line 166-183: After successfully posting the AI reply with
client.chat_postMessage (using channel=channel_id, blocks=blocks,
thread_ts=message.slack_message_id), add a follow-up call to react with a ✅ to
the original message/thread; specifically, invoke the Slack reactions API (e.g.,
client.reactions_add) using the same channel_id and message.slack_message_id
after result is returned, and handle/log any errors (use existing logger) so the
reaction step doesn't break the main flow; keep this insertion immediately after
the result assignment following format_blocks and chat_postMessage.
- Around line 274-285: After posting the "Thinking..." message in the block that
calls client.chat_postMessage (so you have thinking_ts), add a reactions_add
call to add the "eyes" reaction for slash-command flows: call
client.reactions_add(name="eyes", channel=channel_id, timestamp=thinking_ts) and
wrap it in a try/except SlackApiError to log failures with logger.exception; use
the existing thinking_ts, client, SlackApiError and logger symbols so the
reaction is added here (process_ai_query_async already removes the reaction
later).
🧹 Nitpick comments (8)
backend/apps/ai/router.py (1)
48-50: Consider moving imports to module level.Importing
logginginside the function adds minor overhead on each call. Sinceloggingis a standard library module with no circular dependency risk, it's conventional to import at module level.♻️ Suggested refactor
"""Router agent for intent classification.""" import contextlib +import logging from crewai import Agent, Crew, Task from apps.ai.common.intent import Intent from apps.ai.common.llm_config import get_llm from apps.ai.template_loader import env + +logger = logging.getLogger(__name__)Then remove lines 48-50 from the function.
backend/apps/slack/common/question_detector.py (1)
48-54: Variable and log message naming inconsistency after LLM migration.The variable
openai_resultand log message "OpenAI detection failed" are stale after renaming tois_owasp_question_with_llm. This could confuse future maintainers.♻️ Suggested fix
- openai_result = self.is_owasp_question_with_llm(text, context_chunks) + llm_result = self.is_owasp_question_with_llm(text, context_chunks) - if openai_result is None: + if llm_result is None: logger.warning( - "OpenAI detection failed.", + "LLM detection failed.", ) return False - return openai_result + return llm_resultbackend/apps/slack/common/handlers/ai.py (2)
52-52: Consider definingMAX_BLOCK_TEXT_LENGTHat module level.PEP 8 reserves SCREAMING_SNAKE_CASE for module-level constants. Since this value is reused throughout the function and unlikely to change, moving it to module level improves clarity and satisfies the linter.
♻️ Suggested refactor
+# Slack has a limit of 3001 characters per block text +MAX_BLOCK_TEXT_LENGTH = 3000 # Leave some margin for safety + + def format_blocks(text: str) -> list[dict]: """Format AI response text into Slack blocks. ... """ formatted_response = format_ai_response_for_slack(text) - # Slack has a limit of 3001 characters per block text - # Split into multiple blocks if needed - MAX_BLOCK_TEXT_LENGTH = 3000 # Leave some margin for safety - if len(formatted_response) <= MAX_BLOCK_TEXT_LENGTH:
105-131: Consider simplifying redundant validation passes.The code performs three separate validation loops (lines 94-103, 105-118, 120-130) to ensure blocks stay under the limit. Since
format_links_for_slackconverts Markdown[text](url)to Slack<url|text>format, it typically reduces length (or keeps it similar). Two full re-validation passes after the initial split seem excessive.A single validation pass after the initial block construction should suffice, or add a comment explaining why multiple passes are needed.
♻️ Simplified approach
# Add the last block if current_block_text.strip(): - # Final safety check - if still too long, split by character count - if len(current_block_text) > MAX_BLOCK_TEXT_LENGTH: - for i in range(0, len(current_block_text), MAX_BLOCK_TEXT_LENGTH): - chunk = current_block_text[i : i + MAX_BLOCK_TEXT_LENGTH] - if chunk.strip(): - blocks.append(markdown(chunk.strip())) - else: - blocks.append(markdown(current_block_text.strip())) + blocks.append(markdown(current_block_text.strip())) # Final validation: ensure all blocks are under the limit - # This is critical because format_links_for_slack might change text length slightly validated_blocks = [] for block in blocks: block_text = block.get("text", {}).get("text", "") - # Double-check the actual text length in the block if len(block_text) > MAX_BLOCK_TEXT_LENGTH: - # Split this block by character count for i in range(0, len(block_text), MAX_BLOCK_TEXT_LENGTH): chunk = block_text[i : i + MAX_BLOCK_TEXT_LENGTH] if chunk.strip(): validated_blocks.append(markdown(chunk.strip())) else: validated_blocks.append(block) - # One more pass to ensure we didn't miss anything - final_blocks = [] - for block in validated_blocks: - block_text = block.get("text", {}).get("text", "") - if len(block_text) > MAX_BLOCK_TEXT_LENGTH: - for i in range(0, len(block_text), MAX_BLOCK_TEXT_LENGTH): - chunk = block_text[i : i + MAX_BLOCK_TEXT_LENGTH] - if chunk.strip(): - final_blocks.append(markdown(chunk.strip())) - else: - final_blocks.append(block) - - return final_blocks + return validated_blocksbackend/apps/ai/flows/assistant.py (4)
117-124: Redundant condition in greeting detection.The condition
query_lower in simple_greetings or any(query_lower == greeting for greeting in simple_greetings)is redundant—both checks are equivalent. Theinoperator already performs exact matching against list elements.♻️ Simplified condition
is_simple_greeting = ( - ( - query_lower in simple_greetings - or any(query_lower == greeting for greeting in simple_greetings) - ) + query_lower in simple_greetings and not has_question_content and not has_owasp_content )
178-185: Duplicate of module-levelINTENT_TO_AGENTmapping.This mapping duplicates
INTENT_TO_AGENTdefined at lines 40-47. Reuse the existing constant to ensure consistency and reduce maintenance burden.♻️ Suggested fix
- # Map intents to agent creation functions - agent_creators = { - Intent.CHAPTER.value: create_chapter_agent, - Intent.COMMUNITY.value: create_community_agent, - Intent.CONTRIBUTION.value: create_contribution_agent, - Intent.GSOC.value: create_contribution_agent, # GSOC uses contribution agent - Intent.PROJECT.value: create_project_agent, - Intent.RAG.value: create_rag_agent, - } - agents = [] tasks = [] for intent_value in all_intents: - if creator := agent_creators.get(intent_value): + if creator := INTENT_TO_AGENT.get(intent_value): agent = creator(allow_delegation=True)
255-290: Duplicatecontribution_keywordslists.The contribution keywords list is defined twice: once at lines 255-290 for owasp-community channel handling and again at lines 426-446 for fallback contribution detection. Extract to a module-level constant for consistency.
♻️ Suggested refactor
+CONTRIBUTION_KEYWORDS = [ + "contribute", + "contributing", + "contributor", + "contributors", + "get involved", + "getting involved", + # ... rest of keywords +] + INTENT_TO_AGENT = { ... }Then replace both inline lists with
CONTRIBUTION_KEYWORDS.Also applies to: 426-446
327-361: Consider extracting tool invocation helper to reduce duplication.The pattern for unwrapping and invoking crewai tools (checking
__wrapped__,func,run,inspect.unwrap) is duplicated forsuggest_gsoc_channelandsuggest_contribute_channel. Extracting this to a helper function improves maintainability.♻️ Suggested helper
def invoke_tool(tool, fallback_template: str, **template_kwargs): """Invoke a crewai tool, handling various wrapper types.""" func = None result = None if hasattr(tool, "__wrapped__"): func = tool.__wrapped__ elif hasattr(tool, "func"): func = tool.func elif hasattr(tool, "run"): result = tool.run({}) else: func = inspect.unwrap(tool) if func and callable(func): result = func() elif result is None: from apps.ai.common.decorators import render_template result = render_template(fallback_template, **template_kwargs) return resultThen use:
result = invoke_tool( suggest_gsoc_channel, "agents/channel/tools/gsoc.jinja", gsoc_channel_id=OWASP_GSOC_CHANNEL_ID.lstrip("#"), )Also applies to: 375-409
…e long responses - Add validation at multiple levels to detect and block "YES"/"NO" responses from Question Detector - Fix Slack API error for responses exceeding 3001 character limit by splitting into multiple blocks - Improve error handling for response posting failures with user-friendly error messages - Clean up excessive debug logging while keeping essential error and flow logs - Ensure "Thinking..." message is always removed even on errors
- Extract duplicated string literals to constants - Remove mistakenly added .plan/ from .gitignore
- Fix mypy errors: convert implicit Optional types to explicit (str | None) - Fix comparison patterns: use set membership instead of multiple == checks - Fix line length violations: split long lines for better readability - Fix iterable operations: use unpacking instead of list concatenation - Fix .gitignore: ensure proper newline at end of file - Address code quality warnings from ruff linter
rajnisk
force-pushed
the
feature/nestbot-ai-assistant
branch
from
d79d0dd to
13fefce
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@backend/apps/slack/commands/ai.py`:
- Around line 13-15: The handler currently returns silently when query =
command.get("text", "").strip() is empty; instead extract channel_id and user_id
from the incoming payload before the empty-check (reorder the existing
channel_id/user_id extraction to precede the query check) and send an ephemeral
usage hint to the user (use the Slack client method your codebase uses, e.g.,
client.chat_postEphemeral) with a short message explaining the /ai usage; keep
the early-return only after sending the ephemeral and reference the variables
query, command, channel_id and user_id to locate the change.
In `@backend/apps/slack/events/app_mention.py`:
- Around line 59-72: Wrap the django_rq.get_queue("ai").enqueue(...) call in a
try/except block to catch enqueue failures (e.g., Redis errors); on exception,
remove the "eyes" reaction and optionally post a user-facing error message, then
re-raise or log the exception via your existing logger. Specifically, surround
the enqueue invocation that calls process_ai_query_async with try/except, use
the same variables (query, channel_id, message_ts, thread_ts, is_app_mention)
when handling the cleanup, and call the existing reaction-removal/posting
helpers (or use the Slack client there) to ensure the reaction is cleared and
the user is notified if enqueue fails. Ensure the exception is logged with
context for debugging.
In `@backend/apps/slack/events/message_posted.py`:
- Around line 42-81: The message_posted handler currently detects bot mentions
(bot_mentioned) and thus can duplicate work with the app_mention handler (which
calls process_ai_query_async) while message_posted enqueues
generate_ai_reply_if_unanswered; to fix, make message_posted explicitly skip any
message where bot_mentioned is true by returning early (so only app_mention
handles mentions), leave the auth_test/exception handling as-is, and keep
generate_ai_reply_if_unanswered for non-mention messages—update the comment to
state that message_posted ignores mentions and app_mention handles them to avoid
duplicate processing.
In `@backend/apps/slack/services/message_auto_reply.py`:
- Around line 168-175: The raise uses a string literal; assign the error text to
a variable and use that variable in the raise to satisfy linting. In the block
handling ai_response_text/response_str (referencing ai_response_text and
response_str), create a message variable (e.g., err_msg = "Invalid response:
Question Detector output detected"), pass err_msg to logger.error (alongside
channel_id and message.slack_message_id) and then raise ValueError(err_msg)
instead of raising a string literal.
🧹 Nitpick comments (7)
backend/apps/ai/management/commands/test_ai_assistant.py (1)
10-11: Extract response preview length to a named constant.Keeps the preview limit easy to tune and avoids a magic number.
Proposed change
class Command(BaseCommand): help = "Benchmark the NestBot AI Assistant with a set of test queries." + RESPONSE_PREVIEW_LIMIT = 500 @@ - response_preview = response[:500] + "..." if len(response) > 500 else response + response_preview = ( + response[: self.RESPONSE_PREVIEW_LIMIT] + "..." + if len(response) > self.RESPONSE_PREVIEW_LIMIT + else response + )Also applies to: 84-86
backend/apps/slack/common/question_detector.py (1)
48-56: Rename OpenAI‑specific naming to LLM‑neutral.This code now uses
get_llm(), so the variable and log message should be provider‑agnostic to avoid confusion.Proposed change
- openai_result = self.is_owasp_question_with_llm(text, context_chunks) + llm_result = self.is_owasp_question_with_llm(text, context_chunks) - if openai_result is None: + if llm_result is None: logger.warning( - "OpenAI detection failed.", + "LLM-based detection failed.", ) return False - return openai_result + return llm_resultbackend/apps/ai/flows/assistant.py (1)
173-221: Preserve intent order when deduplicating collaborative intents.
set()drops order, but the synthesis task usesagents[-1], so the last agent can change run‑to‑run. Consider ordered dedupe to keep selection deterministic (and verify the intended synthesis agent).Proposed change
- all_intents = [intent, *router_result.get("alternative_intents", [])] - all_intents = list(set(all_intents)) # Deduplicate + all_intents = [intent, *router_result.get("alternative_intents", [])] + all_intents = list(dict.fromkeys(all_intents)) # Deduplicate, preserve orderbackend/apps/slack/common/handlers/ai.py (1)
50-53: Hoist block length limit to module scope.Makes the limit easier to share and avoids a function‑scoped “constant” naming violation.
Proposed change
logger = logging.getLogger(__name__) +MAX_BLOCK_TEXT_LENGTH = 3000 # Leave some margin for safety @@ - MAX_BLOCK_TEXT_LENGTH = 3000 # Leave some margin for safety - if len(formatted_response) <= MAX_BLOCK_TEXT_LENGTH:backend/apps/slack/events/message_posted.py (1)
75-77: Consider narrowing the exception type.While the bare
except Exceptionis flagged by static analysis (BLE001), the fallback behavior (skip auto-reply to be safe) is reasonable for this non-critical path. If you want to address the lint warning, catchSlackApiErrorspecifically since that's the expected failure mode forauth_test().♻️ Optional: Narrow exception handling
+from slack_sdk.errors import SlackApiError + # In the try block: - except Exception: + except SlackApiError: logger.warning("Could not check bot mention, skipping auto-reply to be safe.") returnbackend/apps/slack/commands/ai.py (2)
20-27: Consider trimming verbose inline comments.These comments explaining Slack's limitations are helpful for understanding but could be condensed into a single brief comment or moved to the module docstring.
♻️ Condensed comment
channel_id = command.get("channel_id") user_id = command.get("user_id") - # Slash commands don't have a message TS until we post something, - # but we can add reactions to the last message or just use the trigger_id - # However, it's better to just post an ephemeral message or send a message to the channel. - - # For /ai, we usually want to post to the channel. - # But Slack doesn't provide a message TS for the command itself. - # We'll just post a placeholder or just wait for the async reply. - - # Let's post an ephemeral "Thinking..." message or just go async. + # Slash commands have no message TS; process async and post result directly. import django_rq
35-43: Consider adding error handling around enqueue.Similar to the app_mention handler, if the RQ enqueue fails, the user receives no feedback. For slash commands, consider catching the exception and posting an ephemeral error message.
🛡️ Proposed fix
- django_rq.get_queue("ai").enqueue( - process_ai_query_async, - query=query, - channel_id=channel_id, - message_ts=None, - thread_ts=None, - is_app_mention=False, - user_id=user_id, - ) + try: + django_rq.get_queue("ai").enqueue( + process_ai_query_async, + query=query, + channel_id=channel_id, + message_ts=None, + thread_ts=None, + is_app_mention=False, + user_id=user_id, + ) + except Exception: + logger.exception("Failed to enqueue AI query") + client.chat_postEphemeral( + channel=channel_id, + user=user_id, + text="Sorry, I couldn't process your request right now. Please try again later.", + )Note: You'll need to add
import loggingandlogger = logging.getLogger(__name__)at the module level.
I have carefully followed the contribution guidelines. Kindly let me know if anything is missing or if further changes are needed. |
- Fix empty query feedback: Extract channel_id/user_id before empty check and send ephemeral usage hint in ai.py - Fix enqueue error handling: Wrap enqueue in try/except with cleanup (remove reaction, post error, log) in app_mention.py - Fix duplicate bot mention processing: Skip bot mentions in message_posted.py to avoid duplicate processing - Fix string literal in raise: Use variable instead of string literal in message_auto_reply.py to satisfy linting - Auto-enable assistant: Change default is_nest_bot_assistant_enabled to True and auto-enable for new conversations
Remove the second validation loop that was identical to the first pass. This addresses the cubic-dev-ai suggestion to improve code maintainability by eliminating redundant validation logic.
When bot mention detection fails due to parsing errors (KeyError, TypeError, ValueError), set bot_mentioned = False and continue processing rather than returning early. This prevents valid messages from being silently dropped when event structure parsing fails. Addresses cubic-dev-ai concern about potential message loss.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
rajnisk
dismissed stale reviews from cubic-dev-ai[bot] and coderabbitai[bot]
via
8a1413c
|
Please don't forget to mark it as a ready for review when it's done @rajnisk |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/slack/events/message_posted.py (2)
29-31:⚠️ Potential issue | 🟡 MinorLog message does not match the condition.
The condition checks for
subtypeorbot_id, but the log message also mentionsthread_ts, which is handled separately in lines 33-41.📝 Proposed fix
if event.get("subtype") or event.get("bot_id"): - logger.info("Ignored message due to subtype, bot_id, or thread_ts.") + logger.info("Ignored message due to subtype or bot_id.") return
154-163:⚠️ Potential issue | 🟠 MajorMissing error handling for
client.users_info()API call.The
auth_test()call is properly wrapped withSlackApiErrorhandling (lines 67-90), butusers_info()has no error handling. If the Slack API call fails or returns an unexpected response, the exception propagates and message processing fails silently.🛡️ Proposed fix
try: author = Member.objects.get(slack_user_id=user_id, workspace=conversation.workspace) except Member.DoesNotExist: - user_info = client.users_info(user=user_id) - author = Member.update_data(user_info["user"], conversation.workspace, save=True) - logger.info("Created new member") + try: + user_info = client.users_info(user=user_id) + author = Member.update_data(user_info["user"], conversation.workspace, save=True) + logger.info("Created new member") + except SlackApiError: + logger.exception( + "Failed to fetch user info from Slack API", + extra={"channel_id": channel_id, "user_id": user_id}, + ) + return + except (KeyError, TypeError) as e: + logger.warning( + "Unexpected response structure from users_info", + extra={"user_id": user_id, "error": str(e)}, + ) + return
🧹 Nitpick comments (1)
backend/apps/slack/events/message_posted.py (1)
50-57: Addselect_related('workspace')to avoid an extra database query.The
conversation.workspaceis accessed multiple times (lines 62, 155, 158) which triggers additional queries. Usingselect_relatedwill fetch the workspace in a single query.♻️ Proposed fix
try: - conversation = Conversation.objects.get( + conversation = Conversation.objects.select_related("workspace").get( slack_channel_id=channel_id, is_nest_bot_assistant_enabled=True, )
|
Hey @rajnisk, I really like that you added Google as another LLM provider, it'll allow more people to work on NestBot. I tested a few things like the greeting detection and "Thinking..." message. I think it will be better to split this PR into multiple smaller PRs. That way it'll be easier to test and review. It's up to you how you want to split it. |
Thanks for the review and for testing the changes. |
|
@rudransh-shrivastava @arkid15r Should I create separate issues for each of these and link the PRs to them, or is it fine to keep all four PRs under the same issue?
|
rajnisk
force-pushed
the
feature/nestbot-ai-assistant
branch
from
8a1413c to
88d9762
Compare
github-actions
bot
added
docs
rajnisk
force-pushed
the
feature/nestbot-ai-assistant
branch
from
88d9762 to
8a1413c
Compare
github-actions
bot
removed
docs
|
|
Hi, both approaches sound good to me. For separate issues you may have to wait for the assignment to raise a PR, else it'll be closed automatically. |
3 participants
Proposed change
Resolves #3693
This PR implements fixes for NestBot AI Assistant reaction management, Question Detector output prevention, and Slack block character limit handling, fully aligned with the requirements and behavior described in issue #3693.
Key Fixes
Reaction Management
finally)Greeting Detection
Question Detector Output Prevention
process_ai_query, and message posting)"YES"/"NO"outputs from the Question Detector are never returned as final answersSlack Block Character Limit Handling
invalid_blockserrors by automatically splitting responses exceeding Slack’s 3001-character block limitAdditional Improvements
Improved user-facing error messages for:
Gemini Support (Local Development):
Fixed
ObjectDoesNotExisterror for missing system prompts incore.PromptPrevented OOM issues during heavy agent orchestration by switching to sequential execution
Standardized AI provider configuration (OpenAI & Gemini) using
django-configurationsVerification
finally"YES"/"NO"responses are blocked at all validation levelsChecklist
make check-testlocally — all tests passed