Add multi-turn conversations, chart uploads, and command dispatcher#121
Open
mohit-sheth wants to merge 7 commits into
Open
Add multi-turn conversations, chart uploads, and command dispatcher#121mohit-sheth wants to merge 7 commits into
mohit-sheth wants to merge 7 commits into
Conversation
Replace the static help/greeting fallback with a general-purpose agentic handler that passes free-form natural language queries to the LLM with all MCP tools available. The LLM autonomously decides which Orion tools to call based on the user's question. New components: - ConversationManager: thread-safe, in-memory conversation history keyed by Slack thread, with configurable TTL and rolling window - general_query_analyzer: agentic handler following the pr_analyzer pattern — builds messages with conversation history and calls analyze_with_agentic - GENERAL_QUERY_PROMPT: system prompt guiding the LLM on available tools and Slack formatting - Socket listener now handles thread reply messages for multi-turn follow-ups without requiring @mention Also: - Pin mcp>=1.18.0 in requirements.txt - Fix mcp_config.json to use 127.0.0.1 (IPv4) instead of localhost Assisted-by: Claude Signed-off-by: Mohit Sheth <msheth@redhat.com>
- Use str | None for optional channel_id params (PEP 484 no implicit Optional) - Fix ContextVar type annotation in mcp_interceptors to accept None default - Change _config type from Optional[dict] to dict in telemetry_client - Add type annotation for batch variable in telemetry drain - Add default values for event dict lookups in slack_socket_listener Assisted-by: Claude Signed-off-by: Mohit Sheth <msheth@redhat.com>
Route MCP tool execution through invoke_mcp_tool in the general query analyzer so ImageContent blocks from Orion tools (openshift_report_on, metrics_correlation) are intercepted and uploaded to Slack threads as chart images via files_upload_v2. New components: - ImageCollector: per-request image store using contextvars, collects base64 images from MCP tool results for later Slack upload - Custom tool executor in general_query_analyzer that drives chat_with_tools_async directly instead of analyze_with_agentic, enabling image interception Code quality fixes from review: - Use ContextVar.reset(token) instead of set(None) for proper cleanup - Remove dead text parameter from analyze_general_query - Handle image-only tool results without collector (prevent base64 dump) - Fix MIME extension detection with lookup dict - Modernize typing imports in conversation.py - Update tests to match new executor architecture, add tool routing tests - Update README with free-form queries and multi-turn conversation docs Assisted-by: Claude Signed-off-by: Mohit Sheth <msheth@redhat.com>
Extract the 400-line _process_mention method into focused components: - _run_command: shared wrapper handling ack message, try/except, telemetry emission, and error posting to Slack - _handle_pr_analysis: PR-specific response formatting (section splitting) - _handle_nightly_inspection: nightly regression response formatting - _handle_perf_summary: performance summary arg parsing and multi-message - _handle_general_query: conversation history, image collection, chunking _process_mention is now a ~30-line dispatcher that pattern-matches the command and delegates to the appropriate handler via _run_command. Adding a new command handler no longer requires copying 80 lines of boilerplate — just write a handler method and add one elif clause. Assisted-by: Claude Signed-off-by: Mohit Sheth <msheth@redhat.com>
Add tests for previously untested command handlers: - PR analysis dispatch and response splitting by ==== separator - Performance summary dispatch with multi-message response - General query dispatch with analyzer mock Fix existing tests broken by the dispatcher refactor: - Replace stale greeting test with general query dispatch test - Mock analyzer in submit test to avoid hitting live MCP - Fix error handling test to mock analyzer instead of postMessage Remove trivial edge-case tests that don't add value: - conversation: returns-copy, empty-for-unknown, clear - analyzer: empty LLM result, unknown tool name - listener: PR exception (same _run_command path as nightly), perf summary failure, general query exception Assisted-by: Claude Signed-off-by: Mohit Sheth <msheth@redhat.com>
Tell the LLM to use has_nightly_regressed when users ask about PRs or changes between nightly builds, not just for regression detection. Assisted-by: Claude Signed-off-by: Mohit Sheth <msheth@redhat.com>
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds agentic free-form Slack queries via MCP tools with a new ChangesAgentic Query and Conversation Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Slack
participant Listener as SlackSocketListener
participant Manager as ConversationManager
participant Analyzer as GeneralQueryAnalyzer
participant MCP as MCPToolClient
participant Collector as ImageCollector
Slack->>Listener: app_mention / thread reply event
Listener->>Listener: _should_process_message, _clean_mention_text
Listener->>Manager: append_user_message(channel, thread, text)
Listener->>Analyzer: analyze_general_query(messages, channel_id)
Analyzer->>MCP: chat_with_tools_async(tools, execute_tool)
MCP->>Collector: add_image(base64_data, mime_type, tool_name)
MCP-->>Analyzer: LLM response text
Analyzer-->>Listener: success/message result
Listener->>Manager: append_assistant_message(channel, thread, text)
Listener->>Slack: chat_postMessage / upload chart images
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 6
🧹 Nitpick comments (1)
tests/test_slack_socket_listener.py (1)
141-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new thread-reply routing path.
These tests exercise app mentions, but the PR’s new behavior is unmentioned thread replies. Add cases for active thread replies, unrelated thread replies with no reaction/submission, and expired conversations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_slack_socket_listener.py` around lines 141 - 197, The SlackSocketListener tests currently cover app mentions but not the new thread-reply routing path, so add focused cases that exercise the reply handling logic in SlackSocketListener._process_mention and SlackSocketListener._submit_mention_for_processing. Include tests for an active thread reply that should route into the existing submission/analysis flow, an unrelated thread reply that should be ignored without adding reactions or submitting work, and an expired conversation thread reply that should be rejected or handled as expired. Use the existing mocks for mock_socket_mode_client and mock_web_client to assert the expected chat_postMessage/reaction behavior and that analyze_general_query is or is not called as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bugzooka/integrations/mcp_client.py`:
- Around line 138-152: The MCP image handling in MCPClient is reading the wrong
payload key, so image blocks can be forwarded with empty data. Update the image
branch in the result loop to use the actual `ImageContent` base64 field (`data`)
when calling `collector.add_image`, while still supporting any normalized
fallback shape if present, and keep the MIME type lookup aligned with
`mimeType`/existing normalization in `mcp_client.py`.
In `@bugzooka/integrations/slack_socket_listener.py`:
- Around line 299-315: The `handler()` flow in `slack_socket_listener.py` needs
per-thread serialization because the append → snapshot → analyze → append
sequence can interleave across worker threads and reorder replies. Wrap the full
conversation turn in a lock or queue keyed by `(channel, thread_ts)`, so
`conversation_manager.append_user_message`, `get_messages`,
`anyio.run(analyze_general_query, ...)`, and `append_assistant_message` execute
atomically for each Slack thread.
- Around line 102-110: The thread-reply acceptance path in SlackSocketListener
currently treats any non-empty ConversationManager.get_messages() result as
active, but that check does not honor TTL and can allow expired threads through.
Update the message handling logic in SlackSocketListener to use a TTL-aware
active-conversation check before returning True, ideally by checking the
conversation state through ConversationManager with expiry enforcement or a
dedicated helper before logging “Processing thread reply to active conversation”
and accepting the reply.
- Around line 209-216: The telemetry block in slack_socket_listener is silently
swallowing all exceptions with except/pass, which triggers Ruff S110; update the
best-effort token tracking around get_inference_client() to avoid empty
exception handlers. Either extract this logic into a small telemetry helper that
logs failures at debug level, or catch a specific exception and emit a debug log
while leaving the request flow unchanged. Apply the same fix to the other
duplicated telemetry block mentioned in the review.
- Around line 475-483: Thread replies are being accepted too early in the Slack
listener, causing unrelated messages to get reacted to and queued before
filtering. Update the outer gate in slack_socket_listener’s message handling so
that the app_mention/thread-reply branch only passes when the message is part of
an active conversation, reusing the active-conversation check already used by
_should_process_message() or an equivalent helper. Keep the existing
is_thread_reply detection in place, but combine it with the active-conversation
validation before adding the 👀 reaction or submitting work.
In `@bugzooka/telemetry/telemetry_client.py`:
- Line 19: Telemetry is being enabled too early because `_config` was changed
from a null sentinel to an empty dict, which breaks the `TelemetryClient.emit()`
/ `_config is None` startup guard. Restore the disabled-by-default behavior by
keeping `_config` unset until `start()` initializes `TelemetryClient` state, and
make sure `emit()` only queues events after `start()` has configured the
client/thread so `_run_command()` callers remain no-op before startup.
---
Nitpick comments:
In `@tests/test_slack_socket_listener.py`:
- Around line 141-197: The SlackSocketListener tests currently cover app
mentions but not the new thread-reply routing path, so add focused cases that
exercise the reply handling logic in SlackSocketListener._process_mention and
SlackSocketListener._submit_mention_for_processing. Include tests for an active
thread reply that should route into the existing submission/analysis flow, an
unrelated thread reply that should be ignored without adding reactions or
submitting work, and an expired conversation thread reply that should be
rejected or handled as expired. Use the existing mocks for
mock_socket_mode_client and mock_web_client to assert the expected
chat_postMessage/reaction behavior and that analyze_general_query is or is not
called as appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a3f1da62-87b6-4614-8989-e4b2ef06156d
📒 Files selected for processing (17)
README.mdbugzooka/analysis/general_query_analyzer.pybugzooka/analysis/nightly_regression_analyzer.pybugzooka/analysis/perf_summary_analyzer.pybugzooka/analysis/pr_analyzer.pybugzooka/analysis/prompts.pybugzooka/core/conversation.pybugzooka/integrations/image_collector.pybugzooka/integrations/mcp_client.pybugzooka/integrations/mcp_interceptors.pybugzooka/integrations/slack_socket_listener.pybugzooka/telemetry/telemetry_client.pymcp_config.jsonrequirements.txttests/test_conversation.pytests/test_general_query_analyzer.pytests/test_slack_socket_listener.py
Comment on lines
+102
to
+110
| # Process thread replies to conversations we're already tracking | ||
| if event_type == "message": | ||
| thread_ts = event.get("thread_ts") | ||
| channel = event.get("channel") | ||
| if thread_ts and channel: | ||
| msgs = self.conversation_manager.get_messages(channel, thread_ts) | ||
| if msgs: | ||
| self.logger.debug("Processing thread reply to active conversation") | ||
| return True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor TTL before accepting unmentioned thread replies.
Line 107 uses ConversationManager.get_messages() as the active-conversation check, but that method does not evict expired states. Old threads can still be accepted without a mention, then append_user_message() evicts and starts a fresh query. Add a TTL-aware active-conversation check before returning True.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bugzooka/integrations/slack_socket_listener.py` around lines 102 - 110, The
thread-reply acceptance path in SlackSocketListener currently treats any
non-empty ConversationManager.get_messages() result as active, but that check
does not honor TTL and can allow expired threads through. Update the message
handling logic in SlackSocketListener to use a TTL-aware active-conversation
check before returning True, ideally by checking the conversation state through
ConversationManager with expiry enforcement or a dedicated helper before logging
“Processing thread reply to active conversation” and accepting the reply.
Comment on lines
+299
to
+315
| def handler() -> Dict[str, Any]: | ||
| self.conversation_manager.append_user_message( | ||
| channel, thread_ts, clean_text | ||
| ) | ||
| conversation_messages = self.conversation_manager.get_messages( | ||
| channel, thread_ts | ||
| ) | ||
|
|
||
| analysis_result = anyio.run( | ||
| analyze_general_query, conversation_messages, channel | ||
| ) | ||
| result_text = analysis_result.get("message", "") | ||
|
|
||
| if analysis_result.get("success"): | ||
| self.conversation_manager.append_assistant_message( | ||
| channel, thread_ts, result_text | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Serialize each conversation turn per Slack thread.
These append → snapshot → analyze → append steps are not atomic across worker threads. Two quick replies in the same thread can interleave and append assistant responses out of order. Add a per-(channel, thread_ts) lock or queue around the full handler turn.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bugzooka/integrations/slack_socket_listener.py` around lines 299 - 315, The
`handler()` flow in `slack_socket_listener.py` needs per-thread serialization
because the append → snapshot → analyze → append sequence can interleave across
worker threads and reorder replies. Wrap the full conversation turn in a lock or
queue keyed by `(channel, thread_ts)`, so
`conversation_manager.append_user_message`, `get_messages`,
`anyio.run(analyze_general_query, ...)`, and `append_assistant_message` execute
atomically for each Slack thread.
| _thread: Optional[threading.Thread] = None | ||
| _es_client = None | ||
| _config: Optional[dict] = None | ||
| _config: dict = {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep telemetry disabled until start() runs.
Changing _config to {} bypasses the existing if _config is None guard, so emit() will now queue events even when the telemetry client/thread has never been started. Because Slack commands always call telemetry.emit(...) in _run_command(), that turns telemetry into a best-effort hot path in deployments that expected it to be a no-op and can fill the queue before anything is draining it.
Proposed fix
- _config: dict = {}
+ _config: Optional[dict] = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _config: dict = {} | |
| _config: Optional[dict] = None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bugzooka/telemetry/telemetry_client.py` at line 19, Telemetry is being
enabled too early because `_config` was changed from a null sentinel to an empty
dict, which breaks the `TelemetryClient.emit()` / `_config is None` startup
guard. Restore the disabled-by-default behavior by keeping `_config` unset until
`start()` initializes `TelemetryClient` state, and make sure `emit()` only
queues events after `start()` has configured the client/thread so
`_run_command()` callers remain no-op before startup.
- mcp_client: add fallback for MCP image payload keys (data/mimeType) alongside langchain-normalized keys (base64/mime_type) - conversation: evict expired conversations in get_messages() so stale threads are not accepted for processing - slack_socket_listener: replace silent except/pass with debug log for inference telemetry, gate thread replies with _should_process_message before adding reaction or submitting work - telemetry_client: revert _config to Optional[dict] = None to preserve disabled-by-default guard before start() runs, add asserts for mypy - test_conversation: adjust TTL test to use 1s TTL now that get_messages also evicts expired entries Assisted-by: Claude Signed-off-by: Mohit Sheth <msheth@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add agentic free-form query handling with multi-turn conversation support, chart image uploads from Orion MCP tools, and refactor the mention dispatcher for maintainability.
Review commit-by-commit for clarity — each commit is self-contained.
Commits
1.
c24861f— Add interactive multi-turn conversations via agentic query handlerConversationManager: thread-safe, in-memory conversation history keyed by Slack thread (configurable TTL + rolling window)general_query_analyzer: passes free-form questions to the LLM with all MCP tools available — LLM autonomously decides which Orion tools to callGENERAL_QUERY_PROMPTwith tool guidance for Slack formattingmcp>=1.18.0, fixmcp_config.jsonto use IPv42.
69f6193— Fix pre-existing mypy type errors across codebasestr | Nonefor optionalchannel_idparams (PEP 484)ContextVartype inmcp_interceptors_configtype andbatchannotation intelemetry_client3.
ff14134— Add chart image upload support and code quality improvementsImageCollector: per-request image store usingcontextvars, collects base64 images from MCP tool resultsgeneral_query_analyzerroutes throughinvoke_mcp_tool(not the commons wrapper) to interceptImageContentblocksfiles_upload_v2openshift_report_onandmetrics_correlationnow enabled for chart generationContextVar.reset(token), dead param removal, MIME lookup, typing modernization4.
3443f12— Refactor _process_mention into command dispatcher pattern_run_commandwrapper: shared ack, try/except, telemetry boilerplate_handle_pr_analysis,_handle_nightly_inspection,_handle_perf_summary,_handle_general_query_process_mentionreduced from ~400 lines to ~30 lines of dispatch5.
9197bf1— Add handler tests and fix stale tests for command dispatcher6.
ba87888— Add nightly PR/changes guidance to general query prompthas_nightly_regressedwhen users ask about PRs/changes between nightliesNew files
bugzooka/analysis/general_query_analyzer.pybugzooka/core/conversation.pybugzooka/integrations/image_collector.pytests/test_conversation.pytests/test_general_query_analyzer.pyTest plan
pytest tests/test_general_query_analyzer.py tests/test_conversation.py— 11 tests passpytest tests/test_slack_socket_listener.py— 17 tests pass (requires commons installed)analyze pr,inspect,performance summarycommands still work