Nestbot MVP #2113
Nestbot MVP #2113
Conversation
Summary by CodeRabbit
WalkthroughAdds NestBot Assistant MVP: admin toggle and migration for per-conversation enable flag, OWASP question detection, /ai command and AI handler, app_mention and message event handlers, RQ-backed delayed auto-replies and worker, settings/manifest updates, and accompanying tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/slack/events/__init__.py (1)
1-20: Make configuration lazy and idempotent; skip heavy imports when Slack isn’t configured.Today, you import all event modules unconditionally, then check
SlackConfig.app. That does unnecessary work when Slack isn’t configured and increases the chance of import-time side effects. Also, ifconfigure_slack_events()is called more than once (e.g., dev autoreload), handlers can be registered multiple times.Guard early on
SlackConfig.app, and make the function idempotent using a function attribute to avoid extra globals.Apply:
-def configure_slack_events(): - """Configure Slack events after Django apps are ready.""" - from apps.slack.apps import SlackConfig - from apps.slack.events import ( - app_home_opened, - message_posted, - team_join, - url_verification, - ) - from apps.slack.events.event import EventBase - from apps.slack.events.member_joined_channel import ( - catch_all, - contribute, - gsoc, - project_nest, - ) - - if SlackConfig.app: - EventBase.configure_events() +def configure_slack_events(): + """Configure Slack events after Django apps are ready. + + Idempotent and lazy: + - Early-exit if already configured. + - Early-exit (and avoid importing handlers) when Slack app is not configured. + """ + # Idempotency guard + if getattr(configure_slack_events, "_configured", False): + return + + from apps.slack.apps import SlackConfig + if not SlackConfig.app: + return + + # Import handlers only when needed + from apps.slack.events import ( + app_home_opened, + message_posted, + team_join, + url_verification, + ) + from apps.slack.events.event import EventBase + from apps.slack.events.member_joined_channel import ( + catch_all, + contribute, + gsoc, + project_nest, + ) + + EventBase.configure_events() + configure_slack_events._configured = True
🧹 Nitpick comments (28)
backend/apps/slack/constants.py (2)
25-79: Reduce false positives and harden matching strategy for OWASP_KEYWORDS.Several entries are overly broad (“project”, “event”, “chapter”, “security”, “secure”), which will trigger on non-OWASP conversations and increase noise. Consider:
- Split into two curated sets: OWASP-specific terms (e.g., “owasp”, “zaproxy”, “defectdojo”, “juice shop”, “threat dragon”, “nettacker”, “top 10”) and generic security terms.
- Require at least one OWASP-specific term, or 2+ generic terms, before auto-replying.
- Match with casefolded, word-boundary-aware regex to avoid partial hits (e.g., “csrf” vs “xcsrf123”).
If you want to keep it simple while improving precision, minimally prune the most generic words and add a couple of high-signal variants:
-OWASP_KEYWORDS = { +OWASP_KEYWORDS = { "owasp", - "security", + # generic "security"/"secure" removed to reduce noise "vulnerability", "vulnerabilities", - "zap", + "zap", "zaproxy", "owasp zap", "appsec", "devsecops", "nettacker", - "nest", + # "nest" is ambiguous; consider relying on channel flag instead "threat modeling", + "top 10", "top10", "webgoat", "dependency", - "secure", + # "secure" removed (too broad) "penetration", - "project", - "chapter", - "event", + # "project", "chapter", "event" removed (too broad) "committee", - "defect dojo", + "defect dojo", "defectdojo", "juice shop", "red team", "injection", "xss", "csrf", "authentication", "authorization", + "authorisation", "encryption", "cryptography", "threat", "risk", "assessment", - "code review", + "code review", "static analysis", "dynamic analysis", "firewall", "application security", "web security", "mobile security", "api security", "devops", "secure coding", "security testing", "security tools", "security framework", "security guideline", "security standard", "security best practice", "security policy", "security bug", "security patch", "security update", "security fix", }
25-79: Make OWASP_KEYWORDS immutable to prevent accidental mutation.Define as a frozenset to reflect constant semantics.
-OWASP_KEYWORDS = { +OWASP_KEYWORDS = frozenset({ @@ -} +})backend/apps/slack/templates/commands/ai.jinja (2)
3-3: Tighten grammar for command description.Small copy edit for clarity.
-Use this command to ask questions about OWASP projects, OWASP chapters, and community information using AI-powered knowledge base of OWASP. +Use this command to ask questions about OWASP projects, OWASP chapters, and community information using OWASP’s AI‑powered knowledge base.
6-9: Optionally add a couple more high-signal examples.Examples that include “DefectDojo” and “Threat Dragon” will help users discover supported scope.
• `{{ COMMAND }} What are the OWASP Top 10 vulnerabilities?` • `{{ COMMAND }} How do I contribute to an OWASP project?` • `{{ COMMAND }} Tell me about OWASP ZAP` • `{{ COMMAND }} What is secure coding?` +• `{{ COMMAND }} How do I deploy OWASP DefectDojo?` +• `{{ COMMAND }} What is OWASP Threat Dragon used for?`backend/settings/base.py (1)
145-152: Prefer a single Redis URL and consider a dedicated DB/index for RQ.Using URL keeps cache and queue configs consistent and makes TLS (“rediss://”) flips easier. A separate DB (e.g., /1) avoids key collisions with the cache.
- RQ_QUEUES = { - "default": { - "HOST": REDIS_HOST, - "PORT": 6379, - "PASSWORD": REDIS_PASSWORD, - "DEFAULT_TIMEOUT": 360, - } - } + RQ_QUEUES = { + "default": { + # Use a dedicated DB for RQ (change to /1 if cache uses /0). + "URL": f"redis://:{REDIS_PASSWORD}@{REDIS_HOST}:6379/1", + "DEFAULT_TIMEOUT": 360, + } + }Additionally:
- If Redis requires TLS, switch to rediss://.
- Ensure the /django-rq/ UI is staff-only and disabled in production environments.
backend/settings/urls.py (1)
31-31: Protect and gate the Django RQ dashboard routeExposing the RQ dashboard unconditionally can be risky in production. Although django-rq typically guards views with staff_member_required, please verify that’s true for the pinned version and that auth is enforced end-to-end (reverse proxy, CSRF, etc.). Also consider gating the route by installed app or env to avoid import errors and reduce surface area in non-RQ deployments.
Apply either of the following patterns:
Option A — include only when installed:
- path("django-rq/", include("django_rq.urls")),Add after urlpatterns is defined (outside this range):
from django.conf import settings if "django_rq" in settings.INSTALLED_APPS: urlpatterns.append(path("django-rq/", include("django_rq.urls")))Option B — move under admin namespace for consistency:
- path("django-rq/", include("django_rq.urls")), + path("a/rq/", include("django_rq.urls")),backend/apps/slack/common/handlers/ai.py (4)
16-21: Fix docstring: remove unused parameter referenceThe docstring mentions presentation (EntityPresentation | None), which isn’t in the signature and isn’t used. Remove to avoid confusion.
Args: query (str): The user's question. - presentation (EntityPresentation | None): Configuration for entity presentation.
41-46: Make models configurable via settings with sane defaultsHardcoding "gpt-4o" and "text-embedding-3-small" complicates environment parity and rollouts. Read from Django settings with fallbacks. Also import settings at the top of the module.
Top of file (near imports):
+from django.conf import settingsHere:
- rag_tool = RagTool( - chat_model="gpt-4o", - embedding_model="text-embedding-3-small", - ) + rag_tool = RagTool( + chat_model=getattr(settings, "AI_CHAT_MODEL", "gpt-4o"), + embedding_model=getattr(settings, "AI_EMBEDDING_MODEL", "text-embedding-3-small"), + )
10-10: Put logger to use (or remove it)logger is defined but unused. After adding exception handling in get_blocks, ensure logger.exception is called (as suggested) so this symbol is justified.
26-28: Consider Slack block size limits; chunk long responsesSlack mrkdwn section text caps at 3000 chars. Large AI responses can exceed this. Optional: split the response into multiple blocks (e.g., 2900-char chunks).
Example adaptation inside get_blocks (conceptual):
- if ai_response: - return [markdown(ai_response)] + if ai_response: + return [markdown(chunk) for chunk in _chunk_text(ai_response, 2900)]Helper to add near the bottom of the module:
def _chunk_text(text: str, size: int) -> list[str]: return [text[i : i + size] for i in range(0, len(text), size)]backend/tests/apps/slack/common/handlers/ai_test.py (2)
85-95: Align behavior: either document that failures propagate or handle them in get_blocksprocess_ai_query is tested to raise on failure, yet get_blocks doesn’t currently handle exceptions. If you adopt the suggested try/except in get_blocks, add a test to assert that get_blocks returns error blocks when process_ai_query raises.
Proposed new test (add to this file):
@patch("apps.slack.common.handlers.ai.get_error_blocks") @patch("apps.slack.common.handlers.ai.process_ai_query") def test_get_blocks_handles_exceptions(self, mock_process_ai_query, mock_get_error_blocks): mock_process_ai_query.side_effect = Exception("boom") mock_get_error_blocks.return_value = [{"type": "section", "text": {"type": "mrkdwn", "text": "err"}}] result = get_blocks("What is OWASP?") mock_get_error_blocks.assert_called_once() assert result == mock_get_error_blocks.return_value
132-143: Add explicit test for empty/whitespace-only queriesCurrently we test trimming, but not the empty-input path. If you short-circuit empty queries in get_blocks, add a test to assert no AI call occurs.
Proposed new test:
@patch("apps.slack.common.handlers.ai.process_ai_query") @patch("apps.slack.common.handlers.ai.get_error_blocks") def test_get_blocks_empty_query_short_circuits(self, mock_get_error_blocks, mock_process_ai_query): mock_get_error_blocks.return_value = [] result = get_blocks(" ") mock_process_ai_query.assert_not_called() mock_get_error_blocks.assert_called_once() assert result == []backend/apps/slack/commands/__init__.py (1)
4-6: Optional: document why ai appears first in the listIf ordering matters (e.g., discovery side-effects), add a brief comment to avoid future reordering by linters. If not, consider keeping alphabetical order for consistency.
Example:
from . import ( - ai, + # Keep ai imported so the command is discoverable by CommandBase + ai, board,backend/apps/slack/admin/conversation.py (1)
48-61: Add admin list column and filter for the assistant flagFor ops, it’s useful to scan and filter channels by the assistant status.
list_display = ( "name", "slack_channel_id", "created_at", "total_members_count", + "is_nest_bot_assistant_enabled", ) list_filter = ( "sync_messages", "is_archived", "is_channel", "is_general", "is_im", "is_private", + "is_nest_bot_assistant_enabled", )backend/apps/slack/commands/ai.py (1)
21-23: Harden against missing/whitespacetextand add precise types.Slack typically sends
text, but using.get()avoids KeyError in edge cases and makes tests easier to write. Adding return/param types also clarifies the contract.Apply:
-from apps.slack.commands.command import CommandBase +from typing import Any, Mapping +from apps.slack.commands.command import CommandBase @@ - def render_blocks(self, command: dict): + def render_blocks(self, command: Mapping[str, Any]) -> list[dict]: @@ - return get_blocks( - query=command["text"].strip(), - ) + query = (command.get("text") or "").strip() + return get_blocks(query=query)backend/apps/slack/services/message_auto_reply.py (1)
22-24: Add minimal observability for early returnsQuiet returns make this task hard to debug in prod. Please add debug-level logs that explain why the job exits (not enabled, has replies, empty AI response).
Example:
- if not message.conversation.is_nest_bot_assistant_enabled: - return + if not message.conversation.is_nest_bot_assistant_enabled: + logger.debug("NestBot disabled for conversation %s; skipping auto-reply.", message.conversation_id) + return @@ - if message.thread_replies.exists(): - return + if message.thread_replies.exists(): + logger.debug("Message %s already has replies; skipping auto-reply.", message_id) + return @@ - if not ai_response_text: - return + if not ai_response_text: + logger.debug("AI returned empty response for message %s; skipping post.", message_id) + returnAlso consider annotating the function return type as -> None for clarity.
backend/tests/apps/slack/services/message_auto_reply_test.py (1)
10-10: Consider: centralize the test bot token constantMinor nit: several test modules define TEST_BOT_TOKEN. Consider a shared fixture/constants module to avoid drift.
backend/tests/apps/slack/commands/ai_test.py (1)
127-140: Optional: prefer returning [] over None from render_blocks for uniformityCurrent tests allow None, which is fine because handler checks truthiness. Returning [] can simplify types.
If you adopt this, update the test accordingly.
backend/tests/apps/slack/events/message_posted_test.py (1)
248-292: Strengthen assertions: validate the scheduled delay and deduplicationCurrently only checks that enqueue_in was called. To avoid regressions:
- Assert the first arg is timedelta with the configured minutes (see next comment on configurability).
- Provide job_id to dedupe multiple events for the same message.
Example assertion change:
args, kwargs = mock_queue.enqueue_in.call_args assert isinstance(args[0], timedelta) assert args[1] is generate_ai_reply_if_unanswered assert args[2] == 1 # message.id # If you add job_id: assert kwargs.get("job_id") == "nestbot:auto-reply:1"And adapt the handler accordingly.
backend/apps/slack/events/message_posted.py (1)
42-51: Optional: guard against empty query on mentions to avoid noisy error blocksIf a user mentions the bot with no text, get_blocks("") will likely return an error block. Consider short-circuiting to an ephemeral hint or skip replying.
if bot_mention_string in text: - query = text.replace(bot_mention_string, "").strip() + query = text.replace(bot_mention_string, "").strip() + if not query: + logger.info("Bot mentioned without a query; skipping reply.") + returnbackend/apps/slack/common/question_detector.py (4)
16-19: Normalize OWASP keywords to lowercase once to ensure consistent matchingIf OWASP_KEYWORDS contains any mixed-case entries, direct set intersection may miss matches when
contains_owasp_keywordsis called independently with mixed-case text. Normalize on init.- self.owasp_keywords = OWASP_KEYWORDS + # Normalize keywords to lowercase for consistent case-insensitive matching + self.owasp_keywords = {kw.lower() for kw in OWASP_KEYWORDS}
31-33: Broaden type hint to accept None (tests pass None today)
is_owasp_question(None)is exercised in tests. The implementation already short-circuits safely, but the signature advertisesstronly. Align types to avoid mypy/pylint noise.- def is_owasp_question(self, text: str) -> bool: + def is_owasp_question(self, text: str | None) -> bool: """Check if text contains an OWASP-related question."""
48-57: Make keyword detection robust to case, hyphens, and multi-word phrase boundaries
- Current logic assumes caller lowercases text (true for
is_owasp_questionbut not for direct calls in tests/other modules).- Multi-word matching uses simple substring search; it won’t match common variants like “threat-modeling” or multiple whitespace.
- def contains_owasp_keywords(self, text: str) -> bool: - """Check if text contains OWASP-related keywords.""" - words = re.findall(r"\b\w+\b", text) - text_words = set(words) - - intersection = self.owasp_keywords.intersection(text_words) - if intersection: - return True - - return any(" " in keyword and keyword in text for keyword in self.owasp_keywords) + def contains_owasp_keywords(self, text: str) -> bool: + """Check if text contains OWASP-related keywords (case-insensitive, robust to hyphens).""" + if not text: + return False + text_norm = text.lower() + # Token-level check for single-word keywords + words = re.findall(r"\b\w+\b", text_norm) + if self.owasp_keywords.intersection(words): + return True + # Phrase-level check: allow whitespace or hyphens between words + for kw in (k for k in self.owasp_keywords if " " in k): + phrase_pattern = r"\b" + re.escape(kw).replace(r"\ ", r"[\s\-]+") + r"\b" + if re.search(phrase_pattern, text_norm): + return True + return False
10-10: Remove unused logger or add debug breadcrumbs
loggeris currently unused. Either remove it or add low-volume debug logs at key decision points to aid future triage (e.g., why a message wasn’t treated as a question).backend/tests/apps/slack/common/question_detector_test.py (4)
18-25: Remove unused mock fixture
mock_detector_dependenciespatchesQuestionDetectorbut is never referenced by any test. Dead fixture increases cognitive load.- @pytest.fixture - def mock_detector_dependencies(self): - """Mock any external dependencies if needed.""" - with patch("apps.slack.common.question_detector.QuestionDetector") as mock_class: - mock_instance = Mock() - mock_class.return_value = mock_instance - yield mock_instance
152-163: Avoid relying on internal pattern ordering in testsIndexing
compiled_patterns[0]and[1]couples tests to internal ordering, making harmless refactors brittle.- question_mark_pattern = detector.compiled_patterns[0] - assert question_mark_pattern.search("Is this right?") is not None - - question_word_pattern = detector.compiled_patterns[1] - assert question_word_pattern.search("What is this") is not None - assert question_word_pattern.search("How does it work") is not None + # Validate by behavior, not by internal list order + assert any(p.search("Is this right?") for p in detector.compiled_patterns) + assert any(p.search("What is this") for p in detector.compiled_patterns) + assert any(p.search("How does it work") for p in detector.compiled_patterns)
110-121: Add mixed-case and hyphenation cases for keyword coverageCurrent tests cover lowercase well. Add uppercase/mixed-case and hyphenated phrase variants to guard against regressions and to validate robustness improvements suggested in the detector.
Example additions (new tests):
def test_contains_owasp_keywords_case_and_hyphen_variants(detector): assert detector.contains_owasp_keywords("OWASP Security") is True assert detector.contains_owasp_keywords("ZAP scanner") is True assert detector.contains_owasp_keywords("cross-site scripting") is True # hyphenated phrase assert detector.contains_owasp_keywords("threat-modeling process") is True # hyphenated phrase
164-171: Constants coupling may be brittleThese assertions assume OWASP_KEYWORDS includes “security”, “vulnerability”, and “xss”. If constants evolve, this test may fail even if detection remains adequate. Consider testing via behavior (e.g., that
contains_owasp_keywordsreturns True for sentences containing those words) rather than asserting the constant contents directly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
backend/apps/slack/admin/conversation.py(1 hunks)backend/apps/slack/apps.py(1 hunks)backend/apps/slack/commands/__init__.py(1 hunks)backend/apps/slack/commands/ai.py(1 hunks)backend/apps/slack/common/handlers/ai.py(1 hunks)backend/apps/slack/common/question_detector.py(1 hunks)backend/apps/slack/constants.py(1 hunks)backend/apps/slack/events/__init__.py(1 hunks)backend/apps/slack/events/message_posted.py(1 hunks)backend/apps/slack/migrations/0019_conversation_is_nest_bot_assistant_enabled.py(1 hunks)backend/apps/slack/models/conversation.py(1 hunks)backend/apps/slack/services/__init__.py(1 hunks)backend/apps/slack/services/message_auto_reply.py(1 hunks)backend/apps/slack/templates/commands/ai.jinja(1 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(2 hunks)backend/settings/urls.py(1 hunks)backend/tests/apps/slack/commands/ai_test.py(1 hunks)backend/tests/apps/slack/common/handlers/ai_test.py(1 hunks)backend/tests/apps/slack/common/question_detector_test.py(1 hunks)backend/tests/apps/slack/events/message_posted_test.py(1 hunks)backend/tests/apps/slack/services/message_auto_reply_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
backend/apps/slack/services/message_auto_reply.py (4)
backend/apps/slack/common/handlers/ai.py (1)
process_ai_query(31-46)backend/apps/slack/models/message.py (2)
Message(15-160)text(83-85)backend/apps/ai/agent/tools/rag/rag_tool.py (1)
query(34-63)backend/apps/slack/models/workspace.py (1)
bot_token(37-44)
backend/tests/apps/slack/services/message_auto_reply_test.py (4)
backend/apps/slack/models/conversation.py (1)
Conversation(15-107)backend/apps/slack/models/message.py (2)
Message(15-160)text(83-85)backend/apps/slack/models/workspace.py (2)
Workspace(11-44)bot_token(37-44)backend/apps/slack/services/message_auto_reply.py (1)
generate_ai_reply_if_unanswered(15-37)
backend/tests/apps/slack/commands/ai_test.py (1)
backend/apps/slack/commands/command.py (1)
CommandBase(20-157)
backend/tests/apps/slack/events/message_posted_test.py (3)
backend/apps/slack/events/message_posted.py (1)
MessagePosted(17-77)backend/apps/slack/models/conversation.py (1)
Conversation(15-107)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/slack/events/message_posted.py (7)
backend/apps/slack/common/handlers/ai.py (1)
get_blocks(13-28)backend/apps/slack/common/question_detector.py (2)
QuestionDetector(13-57)is_owasp_question(31-42)backend/apps/slack/events/event.py (1)
EventBase(23-265)backend/apps/slack/models/conversation.py (2)
Conversation(15-107)update_data(85-107)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/slack/models/message.py (2)
Message(15-160)text(83-85)backend/apps/slack/services/message_auto_reply.py (1)
generate_ai_reply_if_unanswered(15-37)
backend/tests/apps/slack/common/handlers/ai_test.py (2)
backend/apps/slack/common/handlers/ai.py (2)
get_error_blocks(49-61)process_ai_query(31-46)backend/apps/ai/agent/tools/rag/rag_tool.py (1)
query(34-63)
backend/apps/slack/common/handlers/ai.py (2)
backend/apps/ai/agent/tools/rag/rag_tool.py (2)
RagTool(13-63)query(34-63)backend/apps/slack/blocks.py (1)
markdown(21-34)
backend/tests/apps/slack/common/question_detector_test.py (2)
backend/apps/slack/common/question_detector.py (4)
QuestionDetector(13-57)is_question(44-46)contains_owasp_keywords(48-57)is_owasp_question(31-42)backend/tests/apps/slack/events/message_posted_test.py (1)
test_init(44-48)
backend/apps/slack/events/__init__.py (1)
backend/apps/slack/events/event.py (2)
EventBase(23-265)configure_events(30-37)
backend/apps/slack/common/question_detector.py (1)
backend/apps/slack/models/message.py (1)
text(83-85)
🔇 Additional comments (12)
backend/apps/slack/services/__init__.py (1)
1-1: LGTM: clear package docstring.No issues. Keeping the package initializer minimal is fine.
backend/pyproject.toml (1)
22-22: django-rq 3.1 is compatible with Django 5.1 and Python 3.13
- PyPI “Verified details” show Requires: Python >=3.8 and classifiers include Python 3.13 support (pypi.org).
- Project “Requirements” state Django (3.2+), and the CHANGELOG v2.10.2 explicitly added Django 5.0 support, with no upper bound thereafter (pypi.org, github.com).
- No known runtime or CI issues targeting newer Django or Python versions; the specifier
^3.1will resolve to the latest 3.x release which maintains these compatibilities.backend/settings/base.py (1)
47-53: LGTM: added django_rq to installed apps.Matches the new dependency and enables the admin UI/hooks when URLs are wired.
backend/tests/apps/slack/common/handlers/ai_test.py (1)
71-83: Good verification of RagTool wiringNice coverage asserting model names and the question passthrough. If you switch to settings-driven models, update this assertion accordingly to read from settings or to assert using the default values via getattr.
backend/apps/slack/commands/__init__.py (1)
5-5: Importing ai ensures the command is discoverableBringing ai into the package makes the command available when CommandBase.configure_commands() runs. This is consistent with the existing pattern here.
backend/apps/slack/admin/conversation.py (1)
30-31: Nice: surfaced the feature flag in adminAdding is_nest_bot_assistant_enabled to the form makes the rollout controllable.
backend/apps/slack/apps.py (1)
28-34: Confirm AI auto-reply delay (1 min vs 2 min)The scheduler in
apps/slack/events/message_posted.pyenqueues the AI reply job withtimedelta(minutes=1), matching the PR description (“within 1 minute”). However, Issue #1969 specifies a default delay of 2 minutes. Please clarify the intended behavior and align the implementation, docs, and tests accordingly.• apps/slack/events/message_posted.py (around line 72):
- django_rq.get_queue("default").enqueue_in( - timedelta(minutes=1), generate_ai_reply_if_unanswered, message.id - ) + django_rq.get_queue("default").enqueue_in( + timedelta(minutes=<1 or 2>), generate_ai_reply_if_unanswered, message.id + )• Update PR description and any documentation/comments that reference “1 minute”
• If tests assume timing behavior, add or update assertions to reflect the chosen delaybackend/apps/slack/models/conversation.py (1)
30-32: LGTM: clear, minimal gate for enabling the NestBot Assistant per channel.Field name, default=False, and verbose_name look good. The migration pairs correctly with this.
backend/apps/slack/migrations/0019_conversation_is_nest_bot_assistant_enabled.py (1)
1-17: Migration matches the model change; no issues.Dependency chain and default align with the model. No data migration needed since default=False is desired.
backend/tests/apps/slack/commands/ai_test.py (1)
18-41: LGTM: Command delegates rendering to get_blocks correctlyThe tests robustly cover trimming, None, exceptions, complex content, and inheritance.
backend/apps/slack/common/question_detector.py (1)
20-25: Question heuristics may produce false positives; verify product intent before rolloutThe “help/advice” patterns (e.g.,
recommend|suggest|advice|opinion) mark statements like “I recommend OWASP ZAP” as questions. Combined with keyword detection, this can trigger unsolicited AI replies to non-questions.Consider one of:
- Require either a question mark or a leading question word for these terms, or
- Gate these terms behind presence of “you/your/please/can” or second-person constructs, or
- Use a higher confidence threshold for auto-replies (e.g., require both a question mark and an OWASP keyword for auto messages; relax for direct mentions).
I can propose concrete regexes once we confirm desired behavior.backend/tests/apps/slack/common/question_detector_test.py (1)
66-72: Confirm intent: statements with advice words counted as questionsThese assertions encode that “I recommend this approach” is a question. If that’s not desired for auto-replies, adjust either tests or detection heuristics per product decision.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/slack/apps.py (1)
38-50: Sanitize Slack payloads before logging to avoid PII/secret leakage.Logging the full event body can expose user content and fields like token/team identifiers. Redact sensitive keys and nested values before logging.
Apply this focused change to error_handler:
- def error_handler(error, body, *args, **kwargs) -> None: # noqa: ARG001 + def error_handler(error, body, *args, **kwargs) -> None: # noqa: ARG001 """Handle Slack application errors. @@ - logger.exception(error, extra={"body": body}) + logger.exception(error, extra={"body": _redact_payload(body)})And add these helpers near the top of the file (outside the changed range):
SENSITIVE_KEYS = { "token", "authed_users", "authorizations", "enterprise_id", "team_id", "api_app_id", "event_context", "client_secret", "client_id", } def _redact_payload(obj): """Recursively redact sensitive fields from Slack payloads.""" if isinstance(obj, dict): return { k: ("***" if k in SENSITIVE_KEYS else _redact_payload(v)) for k, v in obj.items() } if isinstance(obj, list): return [_redact_payload(v) for v in obj] return obj
🧹 Nitpick comments (33)
backend/settings/urls.py (1)
31-31: Django-RQ dashboard is staff-only by default
Verified that all views underdjango_rq.urlsare wrapped with@staff_member_required, which requires an authenticated user withis_active=Trueandis_staff=True—no further permissions are enforced by default(stackoverflow.org.cn, runebook.dev).Recommended hardening options:
- Option A (preferred): mount under the Django admin path to leverage existing admin authentication.
- path("django-rq/", include("django_rq.urls")), + path("admin/rq/", include("django_rq.urls")),- Option B: conditionally include the dashboard only in non-production or behind a feature flag.
If you decide to keep the current public mount, please document the staff-only requirement and consider adding monitoring/alerts for any access attempts.
backend/apps/slack/constants.py (1)
25-79: Consider immutability and false-positive control.
- Make the keyword collection immutable to avoid accidental runtime mutation.
- Several very broad terms (“security framework”, “project”, “event”, “nest”) may spike false positives. If downstream logic doesn’t also require question patterns or OWASP proximity, tune this list or move broad terms behind secondary checks (e.g., require at least one strongly-OWASP token like “owasp”, “zap”, “top 10”, “juice shop”, etc.).
Apply this diff to freeze the set; then evaluate narrowing broad terms based on telemetry:
-OWASP_KEYWORDS = { +OWASP_KEYWORDS = frozenset({ @@ -} +})If the detector supports it, confirm that a question-pattern check (e.g., “who/what/how/?” or endswith ‘?’) is required in addition to keyword presence to avoid unsolicited auto-replies in general “security” discussions.
backend/settings/base.py (1)
145-152: Use a single Redis URL for RQ to align with cache config and avoid type surprises.Passing
values.SecretValueproxies directly asHOST/PASSWORDcan be fragile depending on evaluation timing. UsingURLmirrors your cache config pattern and reduces drift.Apply this diff:
- RQ_QUEUES = { - "default": { - "HOST": REDIS_HOST, - "PORT": 6379, - "PASSWORD": REDIS_PASSWORD, - "DEFAULT_TIMEOUT": 360, - } - } + RQ_QUEUES = { + "default": { + "URL": f"redis://:{REDIS_PASSWORD}@{REDIS_HOST}:6379", + "DEFAULT_TIMEOUT": 360, + } + }If you support nonstandard ports or TLS in some envs, consider adding
REDIS_PORTandREDIS_SSLsettings and reflect them here for parity withCACHES.backend/apps/slack/apps.py (1)
19-26: Prefer truthiness over string comparisons for Slack credentials and consider lazy App creation.Comparing to the literal "None" is brittle and environment-specific. Use truthy checks so empty/undefined/None values all disable the app, and consider deferring App creation until ready() for cleaner initialization.
Example refactor (outside this hunk):
# Simple, robust guard app = App( signing_secret=settings.SLACK_SIGNING_SECRET, token=settings.SLACK_BOT_TOKEN, ) if settings.SLACK_BOT_TOKEN and settings.SLACK_SIGNING_SECRET else NoneOr lazily construct in ready() when credentials are present (and memoize the instance if needed).
backend/apps/slack/admin/conversation.py (1)
30-31: Expose the assistant flag prominently in admin lists for faster ops.Great to include the field in Properties. To streamline testing and rollout, also add it to list_display, list_filter, and make it list_editable so admins can toggle without opening each record.
Suggested diffs (outside this hunk):
@@ list_display = ( "name", "slack_channel_id", "created_at", "total_members_count", + "is_nest_bot_assistant_enabled", ) @@ list_filter = ( "sync_messages", "is_archived", "is_channel", "is_general", "is_im", "is_private", + "is_nest_bot_assistant_enabled", ) + + # Allow quick toggling from the list view (must be in list_display and not first column) + list_editable = ("is_nest_bot_assistant_enabled",)backend/apps/slack/migrations/0019_conversation_is_nest_bot_assistant_enabled.py (1)
12-17: Consider indexing if filtering by this flag is frequent.If event handlers or jobs frequently filter on this boolean (alone or with slack_channel_id), an index can help at scale.
Minimal change to add an index on the boolean:
- field=models.BooleanField(default=False, verbose_name="Is Nest Bot Assistant Enabled"), + field=models.BooleanField(default=False, verbose_name="Is Nest Bot Assistant Enabled", db_index=True),Alternatively, a composite index often performs better for selective lookups:
operations += [ migrations.AddIndex( model_name="conversation", index=models.Index( fields=["slack_channel_id", "is_nest_bot_assistant_enabled"], name="conv_chan_ai_enabled_idx", ), ), ]backend/apps/slack/commands/__init__.py (1)
27-29: Avoid registration at import time; centralize command setup in AppConfig.ready().For consistency with event registration and to prevent duplicate registrations on reload, expose a configure_slack_commands() function here and invoke it from SlackConfig.ready() under an idempotent guard.
Apply this diff:
-if SlackConfig.app: - CommandBase.configure_commands() +def configure_slack_commands(): + """Configure Slack commands after Django apps are ready.""" + if SlackConfig.app: + CommandBase.configure_commands()Then, in apps.slack.apps.SlackConfig.ready(), import and call configure_slack_commands() alongside configure_slack_events(), guarded as shown in that file’s review comment.
backend/apps/slack/commands/ai.py (1)
9-23: Harden against missing 'text' and add return type annotation.Slash commands normally include text, but using .get() avoids KeyError from malformed requests; adding the return type improves clarity.
Apply this diff:
-class Ai(CommandBase): +class Ai(CommandBase): @@ - def render_blocks(self, command: dict): + def render_blocks(self, command: dict) -> list[dict]: @@ - return get_blocks( - query=command["text"].strip(), - ) + return get_blocks(query=command.get("text", "").strip())backend/apps/slack/templates/commands/ai.jinja (1)
1-9: Tighten copy; clarify usage and OWASP focusMinor phrasing/punctuation nits and a clearer usage hint. Suggested copy keeps the same intent and improves readability.
-*Ask OWASP AI Assistant* +*Ask the OWASP AI Assistant* -Use this command to ask questions about OWASP projects, OWASP chapters, and community information using AI-powered knowledge base of OWASP. +Use this command to ask questions about OWASP projects, OWASP chapters, and community information using OWASP’s AI-powered knowledge base. + +_Usage:_ `{{ COMMAND }} <your question>` *Examples:* • `{{ COMMAND }} What are the OWASP Top 10 vulnerabilities?` • `{{ COMMAND }} How do I contribute to an OWASP project?` • `{{ COMMAND }} Tell me about OWASP ZAP` -• `{{ COMMAND }} What is secure coding?` +• `{{ COMMAND }} Where can I find OWASP Secure Coding Practices?`backend/apps/slack/models/conversation.py (1)
30-32: Field looks good; consider adding help_text and (optional) DB indexThe boolean flag is clear and defaults to False. Two small enhancements:
- Add help_text for admin clarity.
- Optional: db_index=True if this flag is frequently filtered in queries, though most lookups seem to fetch by slack_channel_id first.
- is_nest_bot_assistant_enabled = models.BooleanField( - verbose_name="Is Nest Bot Assistant Enabled", default=False - ) + is_nest_bot_assistant_enabled = models.BooleanField( + verbose_name="Is Nest Bot Assistant Enabled", + default=False, + help_text="Enable AI assistant auto-replies and @NestBot features for this conversation.", + db_index=True, + )If you adopt db_index, remember to generate the migration.
backend/apps/slack/common/handlers/ai.py (2)
16-19: Docstring mentions a non-existent parameterThe docstring still references “presentation (EntityPresentation | None)”. Please remove to avoid confusion.
- Args: - query (str): The user's question. - presentation (EntityPresentation | None): Configuration for entity presentation. + Args: + query (str): The user's question.
56-61: Error message tone and consistencyConsider a slightly more actionable error message and consistent spacing around the emoji/markdown for readability.
- markdown( - "⚠️*Sorry, I cannot answer your question.*\n" - "Please try again later or contact support if the issue persists." - ) + markdown( + "⚠️ *Sorry, I couldn’t answer that right now.*\n" + "Please try again, or contact support if the issue persists." + )backend/apps/slack/events/__init__.py (2)
4-16: Side-effect imports may trigger F401; annotate to appease lintersThese imports are for side effects (class registration). Add “# noqa: F401” to each to prevent linter noise.
- from apps.slack.events import ( - app_home_opened, - message_posted, - team_join, - url_verification, - ) + from apps.slack.events import ( # noqa: F401 + app_home_opened, # noqa: F401 + message_posted, # noqa: F401 + team_join, # noqa: F401 + url_verification, # noqa: F401 + ) @@ - from apps.slack.events.member_joined_channel import ( - catch_all, - contribute, - gsoc, - project_nest, - ) + from apps.slack.events.member_joined_channel import ( # noqa: F401 + catch_all, # noqa: F401 + contribute, # noqa: F401 + gsoc, # noqa: F401 + project_nest, # noqa: F401 + )
18-19: Log when Slack app is not configuredA debug/warn helps during local setups where tokens are absent.
- if SlackConfig.app: - EventBase.configure_events() + if SlackConfig.app: + EventBase.configure_events() + else: + import logging + logging.getLogger(__name__).warning("Slack events not configured: Slack app is None")backend/apps/slack/services/message_auto_reply.py (4)
22-27: Skip bot/system messages; avoid replying to threads or editsDepending on upstream filters, this task might still run for bot/system events or thread replies. Add guards to reduce noise.
- if message.thread_replies.exists(): + # Don’t auto-reply if this is a bot/system message, an existing thread, or a reply itself. + if message.raw_data.get("bot_id") or message.parent_message_id: + return + + if message.thread_replies.exists(): return
14-21: Add minimal debug logging for observabilityA couple of debug logs help trace enqueue/run decisions without being noisy.
@job("default") def generate_ai_reply_if_unanswered(message_id: int): """Check if a message is still unanswered and generate AI reply.""" try: message = Message.objects.get(pk=message_id) except Message.DoesNotExist: + logger.debug("Auto-reply skipped: message %s does not exist", message_id) return if not message.conversation.is_nest_bot_assistant_enabled: + logger.debug( + "Auto-reply skipped: assistant disabled for channel %s", + message.conversation.slack_channel_id, + ) return
28-31: Prefer cleaned_text as the query sourceUse the normalized message content (mentions, URLs, emojis removed) to improve retrieval quality.
You’ve already defined Message.cleaned_text; leveraging it here increases RAG precision and reduces prompt noise. Incorporated in the diff above.
32-37: Set unfurl options and guard against long blocksOptional UX improvements:
- Set unfurl_links/unfurl_media to False to avoid noisy previews.
- If you expect long answers, consider truncation or splitting across multiple blocks to respect Slack limits.
- client.chat_postMessage( + client.chat_postMessage( channel=message.conversation.slack_channel_id, blocks=[markdown(ai_response_text)], text=message.text, thread_ts=message.slack_message_id, + unfurl_links=False, + unfurl_media=False, )backend/tests/apps/slack/common/question_detector_test.py (2)
1-1: Consider adding module-level docstring formatWhile the docstring is descriptive, consider following the standard format with a brief one-line summary followed by more details if needed.
-"""Tests for question detector functionality.""" +"""Tests for question detector functionality. + +This module contains test cases for the QuestionDetector class +that identifies OWASP-related questions in text. +"""
117-119: Expand critical use case coverage for threat modelingLine 118 tests threat modeling with lowercase "threat modeling", but based on the relevant code snippet showing the QuestionDetector implementation, multi-word keywords like "threat modeling" need exact phrase matching. Consider adding more edge cases.
assert detector.is_owasp_question("Tell me about application security") is True assert detector.is_owasp_question("I need help with threat modeling") is True + assert detector.is_owasp_question("How do I perform Threat Modeling?") is True + assert detector.is_owasp_question("What is THREAT MODELING process?") is True assert detector.is_owasp_question("Can you recommend secure coding practices?") is Truebackend/tests/apps/slack/events/message_posted_test.py (2)
9-9: Security: Consider using more secure test token formatWhile the S105 warning is suppressed, consider using a more clearly fake token format to avoid any confusion or accidental commits of real tokens.
-TEST_BOT_TOKEN = "xoxb-test-token" # noqa: S105 +TEST_BOT_TOKEN = "xoxb-test-123456789-fake-token-for-testing" # noqa: S105
117-122: Thread safety: sharedbot_user_idacross all eventsWe confirmed that
MessagePostedis only instantiated once at startup viaEventBase.configure_events()and its boundhandleris reused for every incoming message. That means the instance fieldbot_user_id(initialized at line 25 inbackend/apps/slack/events/message_posted.py) is shared across all event invocations and mutated on first use. If your Bolt app handles multiple requests concurrently (e.g., threaded HTTP workers), two handlers could race on checking and setting this attribute, leading to redundantauth_test()calls or even inconsistent reads.To eliminate this potential race condition, consider one of the following refactorings:
- Move the caching into a thread-safe, function-scoped helper—e.g. a module-level
@functools.lru_cachewrapper aroundclient.auth_test().- Always call
client.auth_test()within eachhandle_eventinvocation (no shared state).- Protect the read/write of
self.bot_user_idwith a synchronization primitive (e.g., athreading.Lock) if you must cache on the instance.These changes remove shared mutable state from the event handler and ensure consistent behavior regardless of your server’s concurrency model.
backend/tests/apps/slack/commands/ai_test.py (1)
37-38: Remove redundant instance creationSince the fixture already creates an AI command instance, consider using it instead of creating a new one in each test.
Since you're removing the fixture as suggested above, keep the current pattern of creating instances in each test method.
backend/tests/apps/slack/common/handlers/ai_test.py (1)
132-142: Consider simplifying nested context managersThe nested context managers could be simplified using the
contextlib.ExitStackor by combining patches.def test_get_blocks_strips_whitespace(self): """Test that get_blocks properly strips whitespace from query.""" - with patch("apps.slack.common.handlers.ai.process_ai_query") as mock_process_ai_query: + with ( + patch("apps.slack.common.handlers.ai.process_ai_query") as mock_process_ai_query, + patch("apps.slack.common.handlers.ai.get_error_blocks") as mock_get_error_blocks + ): mock_process_ai_query.return_value = None - with patch("apps.slack.common.handlers.ai.get_error_blocks") as mock_get_error_blocks: - mock_get_error_blocks.return_value = [] - - query_with_whitespace = " What is OWASP? " - get_blocks(query_with_whitespace) - - mock_process_ai_query.assert_called_once_with("What is OWASP?") + mock_get_error_blocks.return_value = [] + + query_with_whitespace = " What is OWASP? " + get_blocks(query_with_whitespace) + + mock_process_ai_query.assert_called_once_with("What is OWASP?")backend/apps/slack/events/message_posted.py (6)
29-31: Reduce log noise for ignored eventsThis will fire for every threaded/bot/subtyped message and can flood logs. Prefer debug level.
- logger.info("Ignored message due to subtype, bot_id, or thread_ts.") + logger.debug("Ignored message due to subtype, bot_id, or thread_ts.")
33-36: Cache bot_user_id to avoid repeated auth_test callsauth_test is an extra network hop per process start and can be rate-limited. Cache the value at module-level so subsequent events don’t hit the API again.
+from django.conf import settings + +_BOT_USER_ID = None @@ - if not self.bot_user_id: - auth_response = client.auth_test() - self.bot_user_id = auth_response.get("user_id") + global _BOT_USER_ID + if not (self.bot_user_id or _BOT_USER_ID): + auth_response = client.auth_test() + self.bot_user_id = _BOT_USER_ID = auth_response.get("user_id") + else: + self.bot_user_id = self.bot_user_id or _BOT_USER_ID
42-51: Guard against empty query after stripping the mentionIf a user only mentions the bot without any text, we’ll post an empty message. Bail out early.
if bot_mention_string in text: query = text.replace(bot_mention_string, "").strip() + if not query: + return + client.chat_postMessage( channel=channel_id, blocks=get_blocks(query=query), text=query, thread_ts=event.get("ts"), ) return
53-61: Tweak logging level and add contextThis path will be common for channels where the assistant is not enabled. Downgrade to debug and include channel_id for triage.
- except Conversation.DoesNotExist: - logger.warning("Conversation not found or assistant not enabled.") + except Conversation.DoesNotExist: + logger.debug( + "Conversation not found or assistant not enabled (channel=%s).", + channel_id, + ) return
71-73: Make auto-reply delay configurable and dedupe queued jobsThe linked issue specifies a configurable delay (default 2 minutes). Hardcoding 1 minute deviates from that and makes tuning harder. Also, Slack events can be retried—use a deterministic job_id to avoid duplicate jobs per message.
- django_rq.get_queue("default").enqueue_in( - timedelta(minutes=1), generate_ai_reply_if_unanswered, message.id - ) + delay_seconds = getattr( + settings, "NEST_BOT_ASSISTANT_REPLY_DELAY_SECONDS", 120 + ) + django_rq.get_queue("default").enqueue_in( + timedelta(seconds=delay_seconds), + generate_ai_reply_if_unanswered, + message.id, + job_id=f"ai-reply:{message.id}", + )If the product decision is indeed “1 minute” (per PR description), set NEST_BOT_ASSISTANT_REPLY_DELAY_SECONDS=60 in settings for clarity and tests.
75-77: Improve diagnostics when Member lookup failsInclude identifiers to help trace data inconsistencies across workspaces.
- except Member.DoesNotExist: - logger.warning("Could not find Member") + except Member.DoesNotExist: + logger.warning( + "Could not find Member (user_id=%s, workspace_id=%s)", + user_id, + conversation.workspace_id if 'conversation' in locals() else None, + ) returnbackend/apps/slack/common/question_detector.py (3)
20-29: Avoid recompiling regexes per instanceThese patterns are static. Compile once at module level to reduce startup overhead and GC pressure when multiple detectors are created.
+# Compile once at module scope +QUESTION_PATTERNS = [ + r"\?", + r"^(what|how|why|when|where|which|who|can|could|would|should|is|are|does|do|did)", + r"(help|explain|tell me|show me|guide|tutorial|example)", + r"(recommend|suggest|advice|opinion)", +] +COMPILED_QUESTION_PATTERNS = [re.compile(p, re.IGNORECASE) for p in QUESTION_PATTERNS] @@ - self.question_patterns = [ - r"\?", - r"^(what|how|why|when|where|which|who|can|could|would|should|is|are|does|do|did)", - r"(help|explain|tell me|show me|guide|tutorial|example)", - r"(recommend|suggest|advice|opinion)", - ] - - self.compiled_patterns = [ - re.compile(pattern, re.IGNORECASE) for pattern in self.question_patterns - ] + self.compiled_patterns = COMPILED_QUESTION_PATTERNS
31-43: Minor: avoid redundant lower() for is_question, keep it for keyword checksis_question uses case-insensitive regex; lowercasing before that is unnecessary. Keep lowercasing for keyword intersection to stay robust.
- text_lower = text.lower().strip() - - is_a_question = self.is_question(text_lower) + text_lower = text.lower().strip() + + is_a_question = self.is_question(text) # compiled with IGNORECASE if not is_a_question: return False return self.contains_owasp_keywords(text_lower)
48-58: Tighten multi-word keyword matching and tokenization
- The word-split handles most cases; multi-word fallbacks are fine but can false-match substrings across punctuation. Use a simple normalized whitespace for multi-word matches.
- words = re.findall(r"\b\w+\b", text) + words = re.findall(r"\b\w+\b", text) text_words = set(words) intersection = self.owasp_keywords.intersection(text_words) if intersection: return True - return any(" " in keyword and keyword in text for keyword in self.owasp_keywords) + normalized_text = re.sub(r"\s+", " ", text) + return any( + (" " in kw) and (kw in normalized_text) + for kw in self.owasp_keywords + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
backend/apps/slack/admin/conversation.py(1 hunks)backend/apps/slack/apps.py(1 hunks)backend/apps/slack/commands/__init__.py(1 hunks)backend/apps/slack/commands/ai.py(1 hunks)backend/apps/slack/common/handlers/ai.py(1 hunks)backend/apps/slack/common/question_detector.py(1 hunks)backend/apps/slack/constants.py(1 hunks)backend/apps/slack/events/__init__.py(1 hunks)backend/apps/slack/events/message_posted.py(1 hunks)backend/apps/slack/migrations/0019_conversation_is_nest_bot_assistant_enabled.py(1 hunks)backend/apps/slack/models/conversation.py(1 hunks)backend/apps/slack/services/__init__.py(1 hunks)backend/apps/slack/services/message_auto_reply.py(1 hunks)backend/apps/slack/templates/commands/ai.jinja(1 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(2 hunks)backend/settings/urls.py(1 hunks)backend/tests/apps/slack/commands/ai_test.py(1 hunks)backend/tests/apps/slack/common/handlers/ai_test.py(1 hunks)backend/tests/apps/slack/common/question_detector_test.py(1 hunks)backend/tests/apps/slack/events/message_posted_test.py(1 hunks)backend/tests/apps/slack/services/message_auto_reply_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
backend/apps/slack/commands/ai.py (2)
backend/apps/slack/commands/command.py (1)
CommandBase(20-157)backend/apps/slack/common/handlers/ai.py (1)
get_blocks(13-28)
backend/apps/slack/common/handlers/ai.py (2)
backend/apps/ai/agent/tools/rag/rag_tool.py (2)
RagTool(13-63)query(34-63)backend/apps/slack/blocks.py (1)
markdown(21-34)
backend/tests/apps/slack/services/message_auto_reply_test.py (4)
backend/apps/slack/models/conversation.py (1)
Conversation(15-107)backend/apps/slack/models/message.py (2)
Message(15-160)text(83-85)backend/apps/slack/models/workspace.py (2)
Workspace(11-44)bot_token(37-44)backend/apps/slack/services/message_auto_reply.py (1)
generate_ai_reply_if_unanswered(15-37)
backend/apps/slack/apps.py (2)
backend/apps/mentorship/apps.py (1)
ready(12-14)backend/apps/slack/events/__init__.py (1)
configure_slack_events(1-19)
backend/apps/slack/services/message_auto_reply.py (4)
backend/apps/slack/common/handlers/ai.py (1)
process_ai_query(31-46)backend/apps/slack/models/message.py (2)
Message(15-160)text(83-85)backend/apps/ai/agent/tools/rag/rag_tool.py (1)
query(34-63)backend/apps/slack/models/workspace.py (1)
bot_token(37-44)
backend/tests/apps/slack/common/question_detector_test.py (1)
backend/apps/slack/common/question_detector.py (4)
QuestionDetector(13-57)is_question(44-46)contains_owasp_keywords(48-57)is_owasp_question(31-42)
backend/apps/slack/events/__init__.py (2)
backend/apps/slack/apps.py (1)
SlackConfig(13-33)backend/apps/slack/events/event.py (2)
EventBase(23-265)configure_events(30-37)
backend/tests/apps/slack/commands/ai_test.py (2)
backend/apps/slack/commands/ai.py (1)
Ai(6-23)backend/apps/slack/commands/command.py (1)
CommandBase(20-157)
backend/tests/apps/slack/events/message_posted_test.py (3)
backend/apps/slack/events/message_posted.py (1)
MessagePosted(17-77)backend/apps/slack/models/conversation.py (1)
Conversation(15-107)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/slack/events/message_posted.py (7)
backend/apps/slack/common/handlers/ai.py (1)
get_blocks(13-28)backend/apps/slack/common/question_detector.py (2)
QuestionDetector(13-57)is_owasp_question(31-42)backend/apps/slack/events/event.py (1)
EventBase(23-265)backend/apps/slack/models/conversation.py (2)
Conversation(15-107)update_data(85-107)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/slack/models/message.py (2)
Message(15-160)text(83-85)backend/apps/slack/services/message_auto_reply.py (1)
generate_ai_reply_if_unanswered(15-37)
backend/tests/apps/slack/common/handlers/ai_test.py (2)
backend/apps/slack/common/handlers/ai.py (2)
get_error_blocks(49-61)process_ai_query(31-46)backend/apps/ai/agent/tools/rag/rag_tool.py (1)
query(34-63)
backend/apps/slack/common/question_detector.py (1)
backend/apps/slack/models/message.py (1)
text(83-85)
🔇 Additional comments (14)
backend/pyproject.toml (1)
22-22: Confirm RQ Version Requirement
- django-rq 3.1 (released August 2, 2025) lists Python 3.13 in its PyPI classifiers, so it advertises support for Python 3.13.
- The project declares support for Django >= 3.2, which includes Django 5.1 in its supported range.
To pin precisely which RQ versions are allowed by django-rq 3.1, please run locally:
pip show django-rqand share the
RequiresorRequires-Distline that specifies therqversion constraint (e.g.rq>=1.14,<2.0or similar).Optional refactor: if compatibility is tight, consider loosening to a minor-pinned spec (e.g.
~3.1.x) to improve lockfile stability across environments.backend/apps/slack/services/__init__.py (1)
1-1: LGTM — minimal, explicit package init.A focused docstring without side effects keeps import-time behavior predictable. No action needed.
backend/settings/base.py (1)
52-52: Enabling django_rq in INSTALLED_APPS — good.Matches the async workflow in this PR; no concerns here.
backend/apps/slack/commands/__init__.py (1)
5-5: LGTM adding ai to the public commands.This ensures the /ai command class is imported and discoverable by CommandBase.
backend/apps/slack/templates/commands/ai.jinja (1)
11-13: Confirm placeholders render correctly in this command contextDouble-check that the template rendering path for commands resolves DIVIDER and FEEDBACK_SHARING_INVITE for command surfaces (not just events/DMs). If these placeholders are event-surface-specific, consider inlining the divider and omitting the invite to avoid empty sections.
Would you like a quick grep script to verify how these placeholders are injected across command templates and events?
backend/apps/slack/services/message_auto_reply.py (1)
1-38: Double-check enqueue delays match product requirementIssue #1969 default mentions 2 minutes; the PR summary mentions “within one minute.” Ensure the scheduler that enqueues this job uses the intended delay and that it’s configurable.
If useful, I can provide a quick repo-wide search script to locate where this job is enqueued and confirm the delay/config source.
backend/tests/apps/slack/common/question_detector_test.py (2)
116-120: Good test coverage for OWASP question detectionThe test cases comprehensively validate the detection of OWASP-related questions with various formats and keywords.
178-195: Excellent use of parametrized testingThe parametrized test with complex real-world OWASP questions provides great coverage for practical scenarios.
backend/tests/apps/slack/events/message_posted_test.py (1)
120-121: Improve test assertion specificityThe test correctly verifies the thread_ts parameter is passed, ensuring replies are threaded properly. Good attention to detail.
backend/tests/apps/slack/services/message_auto_reply_test.py (1)
43-92: Well-structured comprehensive test for success pathThe test thoroughly validates the happy path including thread replies check, AI processing, block generation, and Slack API interaction with correct parameters.
backend/tests/apps/slack/commands/ai_test.py (2)
127-140: Good exception handling testThe test correctly validates that exceptions from the underlying service propagate properly.
164-186: Excellent test for special charactersGood coverage of edge cases with special characters that could potentially cause issues in query processing.
backend/tests/apps/slack/common/handlers/ai_test.py (2)
65-83: Well-structured test for RAG tool integrationExcellent test coverage for the AI query processing with proper verification of both initialization parameters and method calls.
114-131: Good error message validationThe test properly validates the exact error message format that users will see.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
backend/apps/slack/services/message_auto_reply.py (1)
9-9: Avoid double AI invocation and render blocks directly from the answer; use cleaned_text; add posting safeguardsPassing ai_response_text into get_blocks triggers a second AI call inside get_blocks (per the common handler), wasting tokens and producing nonsense. Build the blocks from the computed answer, use cleaned_text as input, re-check for replies right before posting to avoid races, and catch SlackApiError around chat_postMessage.
Apply this diff:
-from apps.slack.common.handlers.ai import get_blocks, process_ai_query +from apps.slack.common.handlers.ai import process_ai_query @@ - ai_response_text = process_ai_query(query=message.text) - if not ai_response_text: - return - - client.chat_postMessage( - channel=message.conversation.slack_channel_id, - blocks=get_blocks(ai_response_text), - text=ai_response_text, - thread_ts=message.slack_message_id, - ) + # Prefer cleaned_text to remove mentions/links/noise before sending to AI. + query_text = (message.cleaned_text or message.text or "").strip() + if not query_text: + return + + ai_response_text = process_ai_query(query=query_text) + if not ai_response_text or not ai_response_text.strip(): + return + + # Re-check to avoid racing with human replies that may have arrived meanwhile. + try: + result = client.conversations_replies( + channel=message.conversation.slack_channel_id, + ts=message.slack_message_id, + limit=1, + ) + if result.get("messages") and result["messages"][0].get("reply_count", 0) > 0: + return + except SlackApiError: + logger.exception("Error re-checking replies for message before posting AI reply") + + blocks = [ + {"type": "section", "text": {"type": "mrkdwn", "text": ai_response_text}} + ] + try: + client.chat_postMessage( + channel=message.conversation.slack_channel_id, + blocks=blocks, + text=ai_response_text, + thread_ts=message.slack_message_id, + ) + except SlackApiError: + logger.exception("Failed to post AI auto-reply for message_id=%s", message_id)Also applies to: 39-48
backend/tests/apps/slack/services/message_auto_reply_test.py (2)
44-95: Update success-path test: stop patching get_blocks and assert inline blocks + double replies checkService no longer calls get_blocks and now re-checks replies before posting.
Apply this diff:
- @patch("apps.slack.services.message_auto_reply.Message.objects.get") - @patch("apps.slack.services.message_auto_reply.WebClient") - @patch("apps.slack.services.message_auto_reply.process_ai_query") - @patch("apps.slack.services.message_auto_reply.get_blocks") + @patch("apps.slack.services.message_auto_reply.Message.objects.get") + @patch("apps.slack.services.message_auto_reply.WebClient") + @patch("apps.slack.services.message_auto_reply.process_ai_query") def test_generate_ai_reply_success( self, - mock_get_blocks, mock_process_ai_query, mock_webclient, mock_message_get, mock_message, ): """Test successful AI reply generation.""" mock_message_get.return_value = mock_message - mock_process_ai_query.return_value = "OWASP is a security organization..." - mock_get_blocks.return_value = [ + ai_answer = "OWASP is a security organization..." + mock_process_ai_query.return_value = ai_answer + expected_blocks = [ { "type": "section", "text": { "type": "mrkdwn", - "text": "OWASP is a security organization...", + "text": ai_answer, }, } ] mock_client = Mock() mock_webclient.return_value = mock_client mock_client.conversations_replies.return_value = {"messages": [{"reply_count": 0}]} generate_ai_reply_if_unanswered(mock_message.id) - mock_client.conversations_replies.assert_called_once_with( - channel=mock_message.conversation.slack_channel_id, - ts=mock_message.slack_message_id, - limit=1, - ) - mock_process_ai_query.assert_called_once_with(query=mock_message.text) - mock_get_blocks.assert_called_once_with("OWASP is a security organization...") + # Called twice: initial check + pre-post race check. + assert mock_client.conversations_replies.call_count == 2 + mock_client.conversations_replies.assert_has_calls( + [ + call( + channel=mock_message.conversation.slack_channel_id, + ts=mock_message.slack_message_id, + limit=1, + ), + call( + channel=mock_message.conversation.slack_channel_id, + ts=mock_message.slack_message_id, + limit=1, + ), + ] + ) + mock_process_ai_query.assert_called_once_with(query=mock_message.text) mock_webclient.assert_called_once_with(token=mock_message.conversation.workspace.bot_token) mock_client.chat_postMessage.assert_called_once_with( channel=mock_message.conversation.slack_channel_id, - blocks=[ - { - "type": "section", - "text": { - "type": "mrkdwn", - "text": "OWASP is a security organization...", - }, - } - ], - text="OWASP is a security organization...", + blocks=expected_blocks, + text=ai_answer, thread_ts=mock_message.slack_message_id, )
135-170: Update Slack API error test: stop patching get_blocks and allow two reply checksOn SlackApiError, the service logs and proceeds; with the pre-post re-check it will attempt the second check and still post. Remove get_blocks patching and relax exact call-count assertions on chat_postMessage or assert twice-called conversations_replies.
Apply this diff:
- @patch("apps.slack.services.message_auto_reply.Message.objects.get") - @patch("apps.slack.services.message_auto_reply.WebClient") - @patch("apps.slack.services.message_auto_reply.process_ai_query") - @patch("apps.slack.services.message_auto_reply.get_blocks") - @patch("apps.slack.services.message_auto_reply.logger") + @patch("apps.slack.services.message_auto_reply.Message.objects.get") + @patch("apps.slack.services.message_auto_reply.WebClient") + @patch("apps.slack.services.message_auto_reply.process_ai_query") + @patch("apps.slack.services.message_auto_reply.logger") def test_generate_ai_reply_slack_api_error( self, mock_logger, - mock_get_blocks, mock_process_ai_query, mock_webclient, mock_message_get, mock_message, ): """Test when Slack API error occurs while checking replies.""" mock_message_get.return_value = mock_message mock_client = Mock() mock_webclient.return_value = mock_client mock_client.conversations_replies.side_effect = SlackApiError("API Error", response=Mock()) - mock_process_ai_query.return_value = "OWASP is a security organization..." - mock_get_blocks.return_value = [ - { - "type": "section", - "text": { - "type": "mrkdwn", - "text": "OWASP is a security organization...", - }, - } - ] + mock_process_ai_query.return_value = "OWASP is a security organization..." generate_ai_reply_if_unanswered(mock_message.id) mock_logger.exception.assert_called_once_with("Error checking for replies for message") mock_process_ai_query.assert_called_once() - mock_client.chat_postMessage.assert_called_once() + # Two attempts to check replies (both error), then post. + assert mock_client.conversations_replies.call_count == 2 + mock_client.chat_postMessage.assert_called_once()
🧹 Nitpick comments (6)
docker-compose/local.yaml (3)
119-124: Consider depending on backend readiness to avoid migration race conditions.Worker starts when cache/db are healthy, but backend applies migrations on startup. If any job fires early (e.g., scheduler heartbeat), it may hit unmigrated DB state. Not severe in dev, but easy to harden.
Option A — add a soft dependency on backend service startup:
depends_on: + backend: + condition: service_started cache: condition: service_healthy db: condition: service_healthyOption B — guard the worker with a migration check (dev-only; avoid running migrations from worker):
- python manage.py rqworker default --with-scheduler + (python manage.py migrate --check || true) && python manage.py rqworker default --with-schedulerIf you add a backend healthcheck, you can switch the above to
condition: service_healthyfor stronger ordering. Verify whether any jobs are enqueued on startup that would touch the DB.
116-118: DRY up duplicate backend build config via YAML anchors.Both backend and worker use the same build context/Dockerfile. Using anchors reduces drift.
Example:
services: backend: + &backend-build build: context: ../backend dockerfile: docker/Dockerfile.local worker: - build: - context: ../backend - dockerfile: docker/Dockerfile.local + build: *backend-build
131-132: Optional: speed up local FS mounts on macOS.If devs are on macOS, consider :delegated/:cached to improve I/O with Docker Desktop.
- - ../backend:/home/owasp + - ../backend:/home/owasp:cachedbackend/apps/slack/MANIFEST.yaml (1)
98-102: Add input-safety guardrails for /ai: should_escape=false shifts sanitization burden to backendWith should_escape set to false, Slack will send unescaped user input to your command handler. That’s fine if your rendering path rigorously sanitizes and only outputs via Slack Blocks, but it increases the risk surface. Either switch this to true or explicitly sanitize in the handler before composing blocks.
Apply this diff if you prefer Slack-side escaping:
- - command: /ai + - command: /ai url: https://nest.owasp.org/integrations/slack/commands/ description: AI-powered OWASP Nest assistant usage_hint: <your question> - should_escape: false + should_escape: truebackend/tests/apps/slack/services/message_auto_reply_test.py (2)
3-3: Import call to assert multiple conversations_replies invocationsTests will need call to assert both the initial and pre-post reply checks.
Apply this diff:
-from unittest.mock import Mock, patch +from unittest.mock import Mock, patch, call
14-43: Add a focused test to verify cleaned_text is used when presentOptional but valuable: ensure mentions/links are stripped before sending to AI.
Add this test:
def test_generate_ai_reply_uses_cleaned_text(self, mocker, mock_message, monkeypatch): mock_get = mocker.patch("apps.slack.services.message_auto_reply.Message.objects.get") mock_client_cls = mocker.patch("apps.slack.services.message_auto_reply.WebClient") mock_proc = mocker.patch("apps.slack.services.message_auto_reply.process_ai_query") # Message with mention + link; cleaned_text fixture behavior: returns stripped content. mock_message.text = "<@U123> What is OWASP? https://owasp.org" type(mock_message).cleaned_text = property(lambda self: "What is OWASP?") mock_get.return_value = mock_message mock_client = Mock() mock_client_cls.return_value = mock_client mock_client.conversations_replies.return_value = {"messages": [{"reply_count": 0}]} mock_proc.return_value = "Answer" generate_ai_reply_if_unanswered(mock_message.id) mock_proc.assert_called_once_with(query="What is OWASP?")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
backend/apps/slack/MANIFEST.yaml(3 hunks)backend/apps/slack/constants.py(1 hunks)backend/apps/slack/services/message_auto_reply.py(1 hunks)backend/tests/apps/slack/common/question_detector_test.py(1 hunks)backend/tests/apps/slack/services/message_auto_reply_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)docker-compose/local.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cspell/custom-dict.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/slack/constants.py
- backend/tests/apps/slack/common/question_detector_test.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/apps/slack/services/message_auto_reply.py (4)
backend/apps/slack/common/handlers/ai.py (1)
process_ai_query(31-46)backend/apps/slack/models/message.py (3)
Message(15-160)ts(88-90)text(83-85)backend/apps/slack/models/workspace.py (1)
bot_token(37-44)backend/apps/ai/agent/tools/rag/rag_tool.py (1)
query(34-63)
backend/tests/apps/slack/services/message_auto_reply_test.py (4)
backend/apps/slack/models/conversation.py (1)
Conversation(15-107)backend/apps/slack/models/message.py (3)
Message(15-160)text(83-85)ts(88-90)backend/apps/slack/models/workspace.py (2)
Workspace(11-44)bot_token(37-44)backend/apps/slack/services/message_auto_reply.py (1)
generate_ai_reply_if_unanswered(16-48)
🔇 Additional comments (3)
docker-compose/local.yaml (1)
112-115: No duplicate RQ scheduler detected — configuration is correct
I ran a repository-wide search for any otherrqworkeror--with-schedulerinvocations and only found the one indocker-compose/local.yaml(lines 112–115). There are no other services or scripts starting an RQ scheduler, so this deployment already enforces a single scheduler. No further changes are required.backend/apps/slack/services/message_auto_reply.py (1)
23-24: LGTM: feature flag respectedEarly exit when the assistant is disabled is correct and keeps the job side-effect free.
backend/tests/apps/slack/services/message_auto_reply_test.py (1)
171-211: No changes needed here; tests cover None/empty AI response pathsThese cases look correct and align with early returns in the service.
backend/settings/base.py
Outdated
| @@ -141,6 +142,15 @@ class Base(Configuration): | |||
| } | |||
| } | |||
|
|
|||
| RQ_QUEUES = { | |||
| "default": { | |||
There was a problem hiding this comment.
Customize early: don't use default queue and use separate DB for rq queues.
There was a problem hiding this comment.
Both is_question and is_owasp_question logic design is questionable. We can't afford manually manage a list of terms indicating question intent and OWASP context. It's not scalable.
There was a problem hiding this comment.
can you suggest alternative for it?
| def __init__(self): | ||
| """Initialize MessagePosted event handler.""" | ||
| self.question_detector = QuestionDetector() | ||
| self.bot_user_id = None |
| text = event.get("text", "") | ||
| bot_mention_string = f"<@{self.bot_user_id}>" | ||
|
|
||
| if bot_mention_string in text: |
There was a problem hiding this comment.
It looks like you need app_mention event instead of this hack.
| ) | ||
|
|
||
| django_rq.get_queue("default").enqueue_in( | ||
| timedelta(minutes=1), generate_ai_reply_if_unanswered, message.id |
There was a problem hiding this comment.
The delay should be a configurable external param.
| return | ||
|
|
||
| try: | ||
| author = Member.objects.get(slack_user_id=user_id, workspace=conversation.workspace) |
There was a problem hiding this comment.
New users may get their questions ignored as NestBot doesn't have them in DB yet. Why not to add them instead?
There was a problem hiding this comment.
So for this I will first try to find the author, if not found we create new user and then we reply to the query
| return | ||
|
|
||
| try: | ||
| client = WebClient(token=message.conversation.workspace.bot_token) |
There was a problem hiding this comment.
Can we use the Slack app object instead of creating a low level client.
| *Examples:* | ||
| • `{{ COMMAND }} What are the OWASP Top 10 vulnerabilities?` | ||
| • `{{ COMMAND }} How do I contribute to an OWASP project?` | ||
| • `{{ COMMAND }} Tell me about OWASP ZAP` |
There was a problem hiding this comment.
How come OWASP ZAP is in this list?
git status suggestions implemented
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
backend/settings/base.py (1)
49-49: Good: dedicated RQ app included.This addresses prior advice to avoid the default queue and wire up RQ early.
backend/tests/apps/slack/events/message_posted_test.py (1)
240-242: Assert the exact enqueue_in arguments (delay, callable, and message id).This verifies the 1/2-minute contract precisely and guards against regressions.
- mock_queue.enqueue_in.assert_called_once() + from datetime import timedelta + from apps.ai.common.constants import QUEUE_RESPONSE_TIME_MINUTES + from apps.slack.services.message_auto_reply import generate_ai_reply_if_unanswered + + mock_queue.enqueue_in.assert_called_once_with( + timedelta(minutes=QUEUE_RESPONSE_TIME_MINUTES), + generate_ai_reply_if_unanswered, + mock_message.id, + )docker-compose/local.yaml (2)
24-26: Fix env var bug: Redis password pulls from host var
DJANGO_REDIS_PASSWORDexpands fromDJANGO_REDIS_HOST, which will set the password to the hostname when unset.- DJANGO_REDIS_PASSWORD: ${DJANGO_REDIS_HOST:-nest-cache-password} + DJANGO_REDIS_PASSWORD: ${DJANGO_REDIS_PASSWORD:-nest-cache-password}
131-132: Standardize Redis host default between backend and workerWorker defaults to
cachewhile backend defaults tonest-cache. This will cause environment-dependent connection failures.- DJANGO_REDIS_HOST: ${DJANGO_REDIS_HOST:-cache} + DJANGO_REDIS_HOST: ${DJANGO_REDIS_HOST:-nest-cache}Optional: prefer the service DNS name everywhere for simplicity.
- DJANGO_REDIS_HOST: ${DJANGO_REDIS_HOST:-nest-cache} + DJANGO_REDIS_HOST: ${DJANGO_REDIS_HOST:-cache}Run to spot similar issues:
#!/bin/bash rg -n -C1 'DJANGO_REDIS_(HOST|PASSWORD):' docker-compose/local.yamlbackend/apps/slack/events/message_posted.py (1)
69-73: Make delay configurable and dedupe scheduled jobsIssue requires a configurable delay (default 2m). Also add a deterministic job_id to avoid duplicates from multiple events.
+ from django.conf import settings - django_rq.get_queue("ai").enqueue_in( - timedelta(minutes=QUEUE_RESPONSE_TIME_MINUTES), - generate_ai_reply_if_unanswered, - message.id, - ) + delay_minutes = getattr( + settings, "NESTBOT_AUTOREPLY_DELAY_MINUTES", QUEUE_RESPONSE_TIME_MINUTES + ) + django_rq.get_queue("ai").enqueue_in( + timedelta(minutes=delay_minutes), + generate_ai_reply_if_unanswered, + message.id, + job_id=f"nestbot:auto-reply:{message.id}", + )Quick checks:
#!/bin/bash # Ensure default is 2 minutes as per spec rg -nC2 'QUEUE_RESPONSE_TIME_MINUTES' backend/apps | sed -n '1,120p' # Ensure no duplicate job scheduling for same message id rg -n 'enqueue_in\(' -nC2 backend/apps | rg -n 'job_id=' -nC1
🧹 Nitpick comments (8)
backend/settings/base.py (1)
144-152: RQ queue config looks solid; consider making timeouts/TTLs tunable and adding a result TTL.For long-running AI calls and safer ops, expose timeout/TTL via env and set a finite result TTL.
RQ_QUEUES = { "ai": { "HOST": REDIS_HOST, "PORT": 6379, "PASSWORD": REDIS_PASSWORD, "DB": 1, - "DEFAULT_TIMEOUT": 360, + "DEFAULT_TIMEOUT": int(os.environ.get("RQ_DEFAULT_TIMEOUT", 360)), + "DEFAULT_RESULT_TTL": int(os.environ.get("RQ_DEFAULT_RESULT_TTL", 600)), } }Also ensure the django-rq dashboard is staff-restricted in urls.
backend/tests/apps/slack/events/message_posted_test.py (2)
259-289: Strengthen assertions for member creation and queueing details.Verify Member.update_data args and the queue name + delay.
- mock_django_rq.get_queue.assert_called_once() + mock_django_rq.get_queue.assert_called_once_with("ai") + from datetime import timedelta + from apps.ai.common.constants import QUEUE_RESPONSE_TIME_MINUTES + from apps.slack.services.message_auto_reply import generate_ai_reply_if_unanswered + mock_queue.enqueue_in.assert_called_once() + # Verify first arg and callable without over-coupling kwargs. + args, kwargs = mock_queue.enqueue_in.call_args + assert args[0] == timedelta(minutes=QUEUE_RESPONSE_TIME_MINUTES) + assert args[1] is generate_ai_reply_if_unanswered + # Ensure we updated/created the member with Slack payload and workspace. + from apps.slack.models.member import Member + mock_member.update_data.assert_called_once()
183-185: Remove unused mock call.auth_test isn’t used by the handler in these tests.
- client = Mock() - client.auth_test.return_value = {"user_id": "U987654"} + client = Mock()backend/apps/slack/events/message_posted.py (2)
28-31: Log message mentions thread_ts but condition doesn't check itTidy the log to match the condition.
- logger.info("Ignored message due to subtype, bot_id, or thread_ts.") + logger.info("Ignored message due to subtype or bot_id.")
61-62: Harden users_info call with SlackApiError handlingNetwork/API failures here will bubble up and drop the event. Catch and log; optionally skip auto-reply if user cannot be resolved.
- except Member.DoesNotExist: - user_info = client.users_info(user=user_id) - author = Member.update_data(user_info["user"], conversation.workspace, save=True) + except Member.DoesNotExist: + try: + user_info = client.users_info(user=user_id) + author = Member.update_data(user_info["user"], conversation.workspace, save=True) + except Exception: + logger.exception("Failed to fetch/create member for user_id=%s", user_id) + returnbackend/apps/slack/events/app_mention.py (3)
3-3: Importrefor mention stripping.Needed for robust fallback parsing below.
import logging +import re
16-20: Guard against missingchannelin event.Avoids a Slack API error on malformed events.
def handle_event(self, event, client): """Handle an incoming app mention event.""" - channel_id = event.get("channel") - text = event.get("text", "") + channel_id = event.get("channel") + if not channel_id: + logger.warning("app_mention missing channel") + return + text = event.get("text", "")
37-41: Use blocks-derived fallback text with get_text
Assign the generated blocks to a variable and pass them into get_text for the text fallback. For example:from apps.slack.utils import get_text reply_blocks = get_blocks(query=query) client.chat_postMessage( channel=channel_id, blocks=reply_blocks, text=get_text(reply_blocks), thread_ts=event.get("ts"), )(get_text is defined in backend/apps/slack/utils.py at line 165)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
backend/apps/ai/common/constants.py(1 hunks)backend/apps/slack/MANIFEST.yaml(4 hunks)backend/apps/slack/events/__init__.py(1 hunks)backend/apps/slack/events/app_mention.py(1 hunks)backend/apps/slack/events/message_posted.py(1 hunks)backend/apps/slack/services/message_auto_reply.py(1 hunks)backend/apps/slack/templates/commands/ai.jinja(1 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(2 hunks)backend/tests/apps/slack/events/message_posted_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)docker-compose/local.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cspell/custom-dict.txt
- backend/apps/slack/templates/commands/ai.jinja
- backend/apps/slack/MANIFEST.yaml
- backend/apps/slack/events/init.py
- backend/apps/slack/services/message_auto_reply.py
- backend/pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (3)
backend/tests/apps/slack/events/message_posted_test.py (3)
backend/apps/slack/events/message_posted.py (2)
MessagePosted(17-73)handle_event(26-73)backend/apps/slack/models/conversation.py (1)
Conversation(15-107)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/slack/events/app_mention.py (1)
backend/apps/slack/events/event.py (1)
EventBase(23-265)
backend/apps/slack/events/message_posted.py (5)
backend/apps/slack/common/question_detector.py (2)
QuestionDetector(13-57)is_owasp_question(31-42)backend/apps/slack/models/conversation.py (2)
Conversation(15-107)update_data(85-107)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/slack/models/message.py (2)
Message(15-160)text(83-85)backend/apps/slack/services/message_auto_reply.py (1)
generate_ai_reply_if_unanswered(16-48)
🔇 Additional comments (3)
backend/apps/slack/events/message_posted.py (1)
58-64: Good: create Member on-the-fly for unknown usersThis addresses earlier feedback about ignoring new users.
backend/apps/slack/events/app_mention.py (2)
11-15: Event wiring looks correct.Subclassing EventBase and setting event_type="app_mention" is appropriate.
31-34: Double-check emptiness after mention-stripping.Prevents sending AI with an empty/mention-only prompt.
Add a unit test where the app_mention contains only a user mention and ensure we return early (no post).
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/apps/slack/events/message_posted.py (2)
69-73: Dedupe scheduled auto-reply jobs to prevent multiple bot replies.
Slack may deliver duplicate events; without a deterministic job_id, you can enqueue multiple identical jobs.- django_rq.get_queue("ai").enqueue_in( + django_rq.get_queue("ai").enqueue_in( timedelta(minutes=QUEUE_RESPONSE_TIME_MINUTES), generate_ai_reply_if_unanswered, message.id, + job_id=f"nestbot:auto-reply:{message.id}", )
69-73: Cross-file bug: enqueued task double-calls the AI and on the wrong text.
generate_ai_reply_if_unansweredcurrently callsprocess_ai_query(...)and then passes that response intoget_blocks(...), which calls the AI again on the AI output. Fix in services file.Apply in backend/apps/slack/services/message_auto_reply.py:
- ai_response_text = process_ai_query(query=message.text) - if not ai_response_text: - return - - client.chat_postMessage( - channel=message.conversation.slack_channel_id, - blocks=get_blocks(ai_response_text), - text=ai_response_text, - thread_ts=message.slack_message_id, - ) + blocks = get_blocks(query=message.text) + if not blocks: + return + client.chat_postMessage( + channel=message.conversation.slack_channel_id, + blocks=blocks, + text=message.text, + thread_ts=message.slack_message_id, + )
🧹 Nitpick comments (3)
backend/apps/slack/events/message_posted.py (3)
28-30: Fix misleading log message (no longer ignores thread_ts here).
The log still mentions thread_ts but this branch only checks subtype/bot_id.- logger.info("Ignored message due to subtype, bot_id, or thread_ts.") + logger.info("Ignored message due to subtype or bot_id.")
32-41: Replace unreachable exception with update count check when marking parent answered.
filter().update()never raisesDoesNotExist. Check the affected row count instead.- if event.get("thread_ts"): - try: - Message.objects.filter( - slack_message_id=event.get("thread_ts"), - conversation__slack_channel_id=event.get("channel"), - ).update(has_replies=True) - except Message.DoesNotExist: - logger.warning("Thread message not found.") - return + if event.get("thread_ts"): + updated = Message.objects.filter( + slack_message_id=event["thread_ts"], + conversation__slack_channel_id=event["channel"], + ).update(has_replies=True) + if updated == 0: + logger.warning("Parent thread message not found to mark as answered.") + return
32-41: Optionally persist replies for analytics/search.
Nice MVP behavior to mark parent answered. If you also want full thread history, persist the reply and associate it to the parent.if event.get("thread_ts"): - updated = Message.objects.filter( + updated = Message.objects.filter( slack_message_id=event["thread_ts"], conversation__slack_channel_id=event["channel"], ).update(has_replies=True) if updated == 0: logger.warning("Parent thread message not found to mark as answered.") + else: + # Persist the reply with parent linkage (best-effort). + parent = Message.objects.filter( + slack_message_id=event["thread_ts"], + conversation__slack_channel_id=event["channel"], + ).first() + if parent: + try: + Message.update_data( + data=event, + conversation=parent.conversation, + author=None, # or resolve author if desired + parent_message=parent, + save=True, + ) + except Exception: + logger.exception("Failed to persist thread reply message.") return
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/slack/events/app_mention.py(1 hunks)backend/apps/slack/events/message_posted.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/slack/events/app_mention.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/slack/events/message_posted.py (5)
backend/apps/slack/common/question_detector.py (2)
QuestionDetector(13-57)is_owasp_question(31-42)backend/apps/slack/models/conversation.py (2)
Conversation(15-107)update_data(85-107)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/slack/models/message.py (2)
Message(15-160)text(83-85)backend/apps/slack/services/message_auto_reply.py (1)
generate_ai_reply_if_unanswered(16-48)
🔇 Additional comments (2)
backend/apps/slack/events/message_posted.py (2)
46-53: Conversation gating looks good.
Correctly restricts behavior to channels with NestBot Assistant enabled.
55-56: Gating on OWASP question is appropriate.
Keeps noise low by early-exiting for non-questions.
Proposed change
Resolves #1969
/aicommand@NestBottag gives instant replyChecklist
make check-testlocally; all checks and tests passed.