feat: add thread-aware conversation support#94
Conversation
This commit improves the Slack bot's agent mode interface with better visual formatting and user experience enhancements: **Configuration Changes (internal/config/config.go):** - Updated default thinking message from "Thinking..." to ":thinking_face: _Thinking..._" with emoji and italic formatting **LLM Agent Prompt Improvements (internal/llm/langchain.go):** - Enhanced agent prompt formatting with visual hierarchy using ">" prefixes for thoughts - Clarified tool invocation instructions with better structure and clearer field descriptions - Improved thought/action formatting to be more readable in Slack **Slack Agent Output Formatting (internal/slack/agentCallbackHandler.go):** - Added comprehensive agent output formatting system with two approaches: - Block Kit formatting for rich Slack messages with proper sections, context blocks, and code formatting - Plain text fallback with emojis, visual separators, and structured metadata display - Implemented intelligent parsing of agent responses to extract: - Thoughts with :brain: emoji and italic formatting - Justifications with :scales: emoji - Actions with :right-facing_fist: emoji - Action inputs formatted as code blocks - Observations with :mag: emoji - Added visual separators (───────────) between metadata and AI responses - Proper handling of markdown headers, code blocks, and structured content - Fallback mechanisms to ensure messages always display properly These changes make the agent's reasoning process more transparent and visually appealing in Slack, helping users better understand what the AI is thinking and doing. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Exclude bin/ directory containing build artifacts - Ignore Python files and requirements.txt - Ignore system.prompt file 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add AddReaction and RemoveReaction methods to UserFrontend interface - Implement reaction methods in SlackClient using Slack API - Add stub implementations for StdioClient - Remove code that deleted thinking messages from SendMessage This enables the bot to use emoji reactions for status indicators instead of sending temporary messages. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace SendMessage thinking indicator with AddReaction emoji - Add thinking_face emoji when processing begins - Remove thinking_face emoji when processing completes or errors - Pass message timestamp through to enable reactions on correct message - Update handleUserPrompt and processLLMResponseAndReply signatures This provides a cleaner UX with emoji reactions instead of temporary messages that need to be deleted. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add participatedThreads map to track threads where bot has responded - Enable bot to respond to messages in threads it has participated in - Make message history thread-aware using "channel:threadTS" keys - Update addToHistory and getContextFromHistory to handle thread context - Track thread participation when bot sends messages This allows the bot to maintain separate conversation contexts for each thread and respond to follow-up messages without requiring @mentions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis update introduces thread-aware message handling and participation tracking in the Slack client, enabling the bot to manage separate histories and respond in threads it has participated in. It also adds Slack reaction management, improves agent output formatting for Slack, updates prompt formatting, and modifies default configuration messages. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Slack
participant Bot
participant LLM
User->>Slack: Sends message (possibly in thread)
Slack->>Bot: Delivers message event
Bot->>Bot: Check if DM or participated in thread
alt New thread or DM
Bot->>Bot: Create new conversation history
else Participated thread
Bot->>Bot: Use thread-specific history
end
Bot->>Slack: Add "thinking_face" reaction
Bot->>LLM: Send context and prompt
LLM-->>Bot: Return response
Bot->>Slack: Remove "thinking_face" reaction
Bot->>Slack: Send formatted response (Block Kit/plain)
Bot->>Bot: Track thread participation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
internal/slack/client.go (1)
296-324: Race condition in message history management.The
messageHistorymap is accessed concurrently without synchronization. This could lead to data races and crashes.Add a mutex to protect message history access:
type Client struct { logger *logging.Logger userFrontend UserFrontend mcpClients map[string]*mcp.Client llmMCPBridge *handlers.LLMMCPBridge llmRegistry *llm.ProviderRegistry cfg *config.Config messageHistory map[string][]Message + historyMutex sync.RWMutex historyLimit int discoveredTools map[string]mcp.ToolInfo participatedThreads map[string]bool }Update the methods to use the mutex:
func (c *Client) addToHistory(channelID, threadTS, role, content string) { historyKey := channelID if threadTS != "" { historyKey = fmt.Sprintf("%s:%s", channelID, threadTS) } + c.historyMutex.Lock() + defer c.historyMutex.Unlock() + history, exists := c.messageHistory[historyKey] // ... rest of the method } func (c *Client) getContextFromHistory(channelID, threadTS string) string { historyKey := channelID if threadTS != "" { historyKey = fmt.Sprintf("%s:%s", channelID, threadTS) } + c.historyMutex.RLock() + defer c.historyMutex.RUnlock() + history, exists := c.messageHistory[historyKey] // ... rest of the method }Also applies to: 347-383
🧹 Nitpick comments (1)
internal/slack/agentCallbackHandler.go (1)
222-324: Consider simplifying the state management in this function.The
formatPlainAgentOutputfunction manages complex state with multiple flags and string builders. While functional, it could benefit from a more structured approach.Consider using a struct to encapsulate the parsing state:
+type agentOutputParser struct { + lines []string + result []string + metadataLines []string + inMetadata bool + currentIndex int +} + +func (p *agentOutputParser) parseNext() bool { + // Parse next line logic +} func formatPlainAgentOutput(text string) string { - lines := strings.Split(text, "\n") - var result []string - var metadataLines []string - inMetadata := false - - for i, line := range lines { - // Complex parsing logic - } + parser := &agentOutputParser{lines: strings.Split(text, "\n")} + for parser.parseNext() { + // Simplified loop + } + return strings.Join(parser.result, "\n") }This would make the function more testable and easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)internal/config/config.go(1 hunks)internal/llm/langchain.go(1 hunks)internal/slack/agentCallbackHandler.go(2 hunks)internal/slack/client.go(13 hunks)internal/slack/stdioClient.go(1 hunks)internal/slack/userFrontend.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
internal/config/**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
Configuration management should be handled in
internal/config/, supporting environment variable overrides
Files:
internal/config/config.go
internal/config/config.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
Add environment variable handling in
config.gowhen adding a new LLM provider
Files:
internal/config/config.go
internal/llm/**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
internal/llm/**/*.go: LLM provider factories and LangChain integration should be implemented ininternal/llm/
Factory pattern should be used ininternal/llm/for provider creation
LangChain gateway should provide a unified interface for LLM providers
Environment variables should override config file settings in LLM provider code
Support both native tools and system prompt-based tools in LLM provider code
Adding a new LLM provider requires creating a factory ininternal/llm/
Implement LangChain integration when adding a new LLM provider
Update provider constants when adding a new LLM provider
Files:
internal/llm/langchain.go
internal/slack/**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
Slack client implementation and message formatting should be in
internal/slack/
Files:
internal/slack/stdioClient.gointernal/slack/userFrontend.gointernal/slack/agentCallbackHandler.gointernal/slack/client.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/slack/**/*.go : Slack client implementation and message formatting should be in `internal/slack/`
.gitignore (3)
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to mcp-servers.json : Adding a new MCP server requires updating mcp-servers.json
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to mcp-servers.json : MCP servers must be configured in mcp-servers.json with command/args or URL
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to mcp-servers.json : Configuration files must support both legacy format and new mcpServers format
internal/config/config.go (1)
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/slack/**/*.go : Slack client implementation and message formatting should be in internal/slack/
internal/llm/langchain.go (5)
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : Support both native tools and system prompt-based tools in LLM provider code
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : Implement LangChain integration when adding a new LLM provider
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Agent mode should use multi-turn conversational interactions via LangChain agents and context-aware tool usage decisions
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : LangChain gateway should provide a unified interface for LLM providers
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Standard mode should use single-prompt interactions with tool descriptions in system prompt and direct JSON tool call parsing and execution
internal/slack/stdioClient.go (2)
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/slack/**/*.go : Slack client implementation and message formatting should be in internal/slack/
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/mcp/**/*.go : Clients should support stdio, HTTP, and SSE transport modes
internal/slack/userFrontend.go (1)
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/slack/**/*.go : Slack client implementation and message formatting should be in internal/slack/
internal/slack/agentCallbackHandler.go (1)
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/slack/**/*.go : Slack client implementation and message formatting should be in internal/slack/
internal/slack/client.go (14)
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/slack/**/*.go : Slack client implementation and message formatting should be in internal/slack/
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/mcp/**/*.go : Component-specific loggers should be used for MCP servers
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/mcp/**/*.go : Enable --mcpdebug for MCP client logs when debugging MCP issues
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/mcp/**/*.go : Clients should support stdio, HTTP, and SSE transport modes
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : Support both native tools and system prompt-based tools in LLM provider code
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : LangChain gateway should provide a unified interface for LLM providers
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : Implement LangChain integration when adding a new LLM provider
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/handlers/**/*.go : Tool handlers and LLM-MCP bridge should be implemented in internal/handlers/
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : Update provider constants when adding a new LLM provider
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : LLM provider factories and LangChain integration should be implemented in internal/llm/
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : Adding a new LLM provider requires creating a factory in internal/llm/
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : Factory pattern should be used in internal/llm/ for provider creation
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/config/config.go : Add environment variable handling in config.go when adding a new LLM provider
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/llm/**/*.go : Environment variables should override config file settings in LLM provider code
🧬 Code Graph Analysis (3)
internal/llm/langchain.go (1)
internal/handlers/llm_mcp_bridge.go (1)
generateToolPrompt(41-113)
internal/slack/agentCallbackHandler.go (1)
internal/slack/formatter/formatter.go (1)
CreateBlockMessage(156-271)
internal/slack/client.go (4)
internal/common/logging/logger.go (1)
Logger(38-43)internal/slack/userFrontend.go (1)
UserFrontend(18-29)internal/handlers/llm_mcp_bridge.go (1)
LLMMCPBridge(29-36)internal/config/config.go (1)
Config(17-27)
🔇 Additional comments (15)
.gitignore (2)
39-42: Re-check the blanket*.py+requirements.txtignore ruleIgnoring every Python source file and the repo-level
requirements.txtwill hide any tooling, migration scripts, or CI helpers that should be version-controlled.
If Python is never part of this repository, that’s fine, but please double-check:
- Are there already-tracked
.pyfiles (or a requirements file) that will suddenly become untracked?- Could future contributors need to commit Python code (e.g. release scripts, code-gen, lint hooks)?
If the answer to either question is “yes”, scope the rule more narrowly – for example, ignore only a specific tooling directory:
-*.py -requirements.txt +tools/**/*.py +tools/requirements.txt
36-38: Addingbin/to ignore list looks goodBuild artifacts should not be committed; the new rule cleanly excludes them.
internal/config/config.go (1)
244-244: LGTM! Enhanced default thinking message improves user experience.The addition of the
:thinking_face:emoji and markdown italics creates a more engaging default message that aligns with the reaction-based feedback mechanisms implemented elsewhere in the Slack integration.internal/llm/langchain.go (1)
212-212: LGTM! Prompt format consistency improvement.Adding the
"> "prefix to "Thought:" lines in the prompt instructions creates consistency with the agent output parsing logic implemented ininternal/slack/agentCallbackHandler.go, which expects this format when processing agent responses.Also applies to: 222-222
internal/slack/stdioClient.go (1)
128-136: LGTM! Appropriate stub implementation for stdio client.The
AddReactionandRemoveReactionmethods provide suitable debug output for the stdio client, which is consistent with the testing/development nature of this client implementation. The methods correctly implement theUserFrontendinterface.internal/slack/userFrontend.go (3)
27-28: LGTM! Interface extension for reaction support.The addition of
AddReactionandRemoveReactionmethods to theUserFrontendinterface properly supports the thread-aware conversation enhancements described in the PR objectives.
126-146: LGTM! Solid implementation of reaction methods.The
AddReactionandRemoveReactionimplementations include:
- Proper use of context-aware Slack API calls
- Comprehensive error handling with logging
- Appropriate debug logging for successful operations
- Consistent error message formatting
187-187: LGTM! Simplified variable declaration.The direct assignment to the return values from
PostMessageis cleaner than the previous implementation.internal/slack/agentCallbackHandler.go (1)
21-23: LGTM! Appropriate use of plain formatting.Using
formatPlainAgentOutputis a good choice as it provides enhanced readability while letting theSendMessagefunction handle Block Kit formatting decisions.internal/slack/client.go (6)
28-37: LGTM! Clean implementation of thread participation tracking.The
participatedThreadsmap with the composite key format is a good approach for tracking bot participation across channels and threads.
248-248: Good separation of concerns between message and thread timestamps.The addition of
messageTSparameter enables precise reaction management on the triggering message while maintaining thread context withthreadTS.Also applies to: 285-285, 386-386
255-286: Well-structured thread participation logic.The message handling correctly processes messages in both DMs and participated threads, with clear logging and proper timestamp handling for both reactions and threading.
296-324: Thread-aware history management looks good.The composite key approach effectively separates conversation contexts between threads while maintaining backward compatibility for non-threaded messages.
395-399: Good implementation of visual feedback with thinking reactions.The thinking reaction is properly managed across all code paths, providing clear visual feedback to users while maintaining thread participation tracking.
Also applies to: 419-420, 446-447, 456-459
464-464: Consistent thread-aware implementation in response processing.The method properly maintains thread context throughout the LLM response pipeline and correctly manages reactions and thread participation.
Also applies to: 519-520, 537-537, 547-547, 557-561
| // formatAgentOutput processes the agent output to create a Block Kit message | ||
| func formatAgentOutput(text string) string { | ||
| lines := strings.Split(text, "\n") | ||
| var blocks []slack.Block | ||
| var contextElements []slack.MixedElement | ||
| var currentSection strings.Builder | ||
| inCodeBlock := false | ||
| codeBlockContent := strings.Builder{} | ||
|
|
||
| // Helper function to add context block if elements exist | ||
| addContextBlock := func() { | ||
| if len(contextElements) > 0 { | ||
| blocks = append(blocks, slack.NewContextBlock("", contextElements...)) | ||
| contextElements = []slack.MixedElement{} | ||
| } | ||
| } | ||
|
|
||
| // Helper function to add section block if content exists | ||
| addSectionBlock := func() { | ||
| if currentSection.Len() > 0 { | ||
| blocks = append(blocks, slack.NewSectionBlock( | ||
| slack.NewTextBlockObject("mrkdwn", currentSection.String(), false, false), | ||
| nil, nil, | ||
| )) | ||
| currentSection.Reset() | ||
| } | ||
| } | ||
|
|
||
| for _, line := range lines { | ||
| trimmedLine := strings.TrimSpace(line) | ||
|
|
||
| // Check for code blocks | ||
| if strings.HasPrefix(line, "```") { | ||
| if inCodeBlock { | ||
| // End of code block | ||
| addSectionBlock() // Add any pending section | ||
| blocks = append(blocks, slack.NewSectionBlock( | ||
| slack.NewTextBlockObject("mrkdwn", "```\n"+codeBlockContent.String()+"\n```", false, false), | ||
| nil, nil, | ||
| )) | ||
| codeBlockContent.Reset() | ||
| inCodeBlock = false | ||
| } else { | ||
| // Start of code block | ||
| addSectionBlock() // Add any pending section | ||
| inCodeBlock = true | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| if inCodeBlock { | ||
| if codeBlockContent.Len() > 0 { | ||
| codeBlockContent.WriteString("\n") | ||
| } | ||
| codeBlockContent.WriteString(line) | ||
| continue | ||
| } | ||
|
|
||
| // Process thoughts and metadata as context blocks | ||
| if strings.HasPrefix(trimmedLine, "> Thought:") || strings.HasPrefix(trimmedLine, "Thought:") { | ||
| addSectionBlock() // Add any pending section | ||
| thoughtContent := strings.TrimPrefix(trimmedLine, "> ") | ||
| thoughtContent = strings.TrimPrefix(thoughtContent, "Thought:") | ||
| contextElements = append(contextElements, | ||
| slack.NewTextBlockObject("mrkdwn", "_Thought:_ "+strings.TrimSpace(thoughtContent), false, false)) | ||
| } else if strings.HasPrefix(trimmedLine, "Justification:") { | ||
| justContent := strings.TrimPrefix(trimmedLine, "Justification:") | ||
| contextElements = append(contextElements, | ||
| slack.NewTextBlockObject("mrkdwn", "_Justification:_ "+strings.TrimSpace(justContent), false, false)) | ||
| } else if strings.HasPrefix(trimmedLine, "Action:") { | ||
| actionContent := strings.TrimPrefix(trimmedLine, "Action:") | ||
| contextElements = append(contextElements, | ||
| slack.NewTextBlockObject("mrkdwn", "_Action:_ `"+strings.TrimSpace(actionContent)+"`", false, false)) | ||
| } else if strings.HasPrefix(trimmedLine, "Action Input:") { | ||
| // Action Input often contains JSON, keep it in code formatting | ||
| inputContent := strings.TrimPrefix(trimmedLine, "Action Input:") | ||
| contextElements = append(contextElements, | ||
| slack.NewTextBlockObject("mrkdwn", "_Action Input:_ `"+strings.TrimSpace(inputContent)+"`", false, false)) | ||
| } else if strings.HasPrefix(trimmedLine, "Observation:") { | ||
| // Add context blocks before observation | ||
| addContextBlock() | ||
| obsContent := strings.TrimPrefix(trimmedLine, "Observation:") | ||
| contextElements = append(contextElements, | ||
| slack.NewTextBlockObject("mrkdwn", "_Observation:_ "+strings.TrimSpace(obsContent), false, false)) | ||
| } else if strings.HasPrefix(trimmedLine, "AI:") { | ||
| // Finish any pending context blocks | ||
| addContextBlock() | ||
| // Remove the "AI: " prefix and treat as regular content | ||
| content := strings.TrimSpace(strings.TrimPrefix(trimmedLine, "AI:")) | ||
| if content != "" { | ||
| // Check if this looks like a header (starts with # or is in all caps) | ||
| if strings.HasPrefix(content, "# ") || strings.HasPrefix(content, "## ") { | ||
| headerText := strings.TrimLeft(content, "# ") | ||
| addSectionBlock() // Add any pending section | ||
| blocks = append(blocks, slack.NewHeaderBlock( | ||
| slack.NewTextBlockObject("plain_text", headerText, false, false), | ||
| )) | ||
| } else { | ||
| if currentSection.Len() > 0 { | ||
| currentSection.WriteString("\n") | ||
| } | ||
| currentSection.WriteString(content) | ||
| } | ||
| } | ||
| } else { | ||
| // Regular content | ||
| addContextBlock() // Add any pending context blocks | ||
|
|
||
| // Check for markdown headers | ||
| if strings.HasPrefix(trimmedLine, "# ") { | ||
| headerText := strings.TrimPrefix(trimmedLine, "# ") | ||
| addSectionBlock() // Add any pending section | ||
| blocks = append(blocks, slack.NewHeaderBlock( | ||
| slack.NewTextBlockObject("plain_text", headerText, false, false), | ||
| )) | ||
| } else if strings.HasPrefix(trimmedLine, "## ") { | ||
| headerText := strings.TrimPrefix(trimmedLine, "## ") | ||
| addSectionBlock() // Add any pending section | ||
| blocks = append(blocks, slack.NewHeaderBlock( | ||
| slack.NewTextBlockObject("plain_text", headerText, false, false), | ||
| )) | ||
| } else if strings.HasPrefix(trimmedLine, "### ") { | ||
| // For smaller headers, use bold text in section | ||
| headerText := strings.TrimPrefix(trimmedLine, "### ") | ||
| if currentSection.Len() > 0 { | ||
| currentSection.WriteString("\n") | ||
| } | ||
| currentSection.WriteString("*" + headerText + "*") | ||
| } else { | ||
| // Regular line | ||
| if line != "" || currentSection.Len() > 0 { | ||
| if currentSection.Len() > 0 && line != "" { | ||
| currentSection.WriteString("\n") | ||
| } | ||
| currentSection.WriteString(line) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Add any remaining content | ||
| addContextBlock() | ||
| addSectionBlock() | ||
|
|
||
| // If we have blocks, create a Block Kit message | ||
| if len(blocks) > 0 { | ||
| // Add a divider between thoughts and response if we have context | ||
| hasContext := false | ||
| for _, block := range blocks { | ||
| if _, ok := block.(*slack.ContextBlock); ok { | ||
| hasContext = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if hasContext { | ||
| // Find where to insert divider (after last context block) | ||
| dividerIndex := -1 | ||
| for i, block := range blocks { | ||
| if _, ok := block.(*slack.ContextBlock); ok { | ||
| dividerIndex = i | ||
| } | ||
| } | ||
|
|
||
| if dividerIndex >= 0 && dividerIndex < len(blocks)-1 { | ||
| // Insert divider after the last context block | ||
| newBlocks := make([]slack.Block, 0, len(blocks)+1) | ||
| newBlocks = append(newBlocks, blocks[:dividerIndex+1]...) | ||
| newBlocks = append(newBlocks, slack.NewDividerBlock()) | ||
| newBlocks = append(newBlocks, blocks[dividerIndex+1:]...) | ||
| blocks = newBlocks | ||
| } | ||
| } | ||
|
|
||
| // Convert blocks to JSON format | ||
| blockData := map[string]interface{}{ | ||
| "text": text, // Fallback text | ||
| "blocks": blocks, | ||
| } | ||
|
|
||
| // Marshal to JSON | ||
| jsonBytes, err := json.Marshal(blockData) | ||
| if err != nil { | ||
| // Fallback to plain formatting | ||
| return formatPlainAgentOutput(text) | ||
| } | ||
|
|
||
| return string(jsonBytes) | ||
| } | ||
|
|
||
| // Fallback to formatted text if no blocks were created | ||
| return formatPlainAgentOutput(text) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring this complex function for better maintainability.
The formatAgentOutput function is quite complex with multiple responsibilities:
- Parsing different line types (metadata, code blocks, headers)
- Managing multiple state variables
- Building Slack Block Kit structures
- Handling edge cases
Consider breaking this down into smaller, focused functions:
+// parseAgentLine processes a single line and returns its type and content
+func parseAgentLine(line string) (lineType string, content string) {
+ // Line parsing logic here
+}
+
+// buildContextElements creates context block elements from metadata
+func buildContextElements(metadataLines []string) []slack.MixedElement {
+ // Context building logic here
+}
+
func formatAgentOutput(text string) string {
- // Current complex implementation
+ // Simplified orchestration using helper functions
}This would improve readability, testability, and maintainability.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/slack/agentCallbackHandler.go from lines 28 to 220, the
formatAgentOutput function is overly complex, handling parsing, state
management, and Slack Block Kit construction all in one. Refactor by extracting
distinct responsibilities into smaller helper functions, such as separate
parsers for metadata lines, code block handling, header processing, and block
assembly. This modularization will improve readability, simplify testing, and
make maintenance easier.
| // trackThreadParticipation marks a thread as participated by the bot | ||
| func (c *Client) trackThreadParticipation(channelID, threadTS string) { | ||
| if threadTS != "" { | ||
| threadKey := fmt.Sprintf("%s:%s", channelID, threadTS) | ||
| c.participatedThreads[threadKey] = true | ||
| c.logger.DebugKV("Tracked thread participation", "channel", channelID, "thread", threadTS) | ||
| } | ||
| } | ||
|
|
||
| // hasParticipatedInThread checks if the bot has participated in a thread | ||
| func (c *Client) hasParticipatedInThread(channelID, threadTS string) bool { | ||
| if threadTS == "" { | ||
| return false | ||
| } | ||
| threadKey := fmt.Sprintf("%s:%s", channelID, threadTS) | ||
| return c.participatedThreads[threadKey] | ||
| } |
There was a problem hiding this comment.
Potential race condition in thread participation tracking.
The participatedThreads map is accessed concurrently from multiple goroutines without synchronization. Consider adding a mutex to protect map access.
type Client struct {
logger *logging.Logger
userFrontend UserFrontend
mcpClients map[string]*mcp.Client
llmMCPBridge *handlers.LLMMCPBridge
llmRegistry *llm.ProviderRegistry
cfg *config.Config
messageHistory map[string][]Message
historyLimit int
discoveredTools map[string]mcp.ToolInfo
participatedThreads map[string]bool
+ threadMutex sync.RWMutex
}And update the methods:
func (c *Client) trackThreadParticipation(channelID, threadTS string) {
if threadTS != "" {
threadKey := fmt.Sprintf("%s:%s", channelID, threadTS)
+ c.threadMutex.Lock()
c.participatedThreads[threadKey] = true
+ c.threadMutex.Unlock()
c.logger.DebugKV("Tracked thread participation", "channel", channelID, "thread", threadTS)
}
}
func (c *Client) hasParticipatedInThread(channelID, threadTS string) bool {
if threadTS == "" {
return false
}
threadKey := fmt.Sprintf("%s:%s", channelID, threadTS)
+ c.threadMutex.RLock()
+ participated := c.participatedThreads[threadKey]
+ c.threadMutex.RUnlock()
- return c.participatedThreads[threadKey]
+ return participated
}📝 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.
| // trackThreadParticipation marks a thread as participated by the bot | |
| func (c *Client) trackThreadParticipation(channelID, threadTS string) { | |
| if threadTS != "" { | |
| threadKey := fmt.Sprintf("%s:%s", channelID, threadTS) | |
| c.participatedThreads[threadKey] = true | |
| c.logger.DebugKV("Tracked thread participation", "channel", channelID, "thread", threadTS) | |
| } | |
| } | |
| // hasParticipatedInThread checks if the bot has participated in a thread | |
| func (c *Client) hasParticipatedInThread(channelID, threadTS string) bool { | |
| if threadTS == "" { | |
| return false | |
| } | |
| threadKey := fmt.Sprintf("%s:%s", channelID, threadTS) | |
| return c.participatedThreads[threadKey] | |
| } | |
| // --- add import at top of file if not already present --- | |
| // import "sync" | |
| type Client struct { | |
| logger *logging.Logger | |
| userFrontend UserFrontend | |
| mcpClients map[string]*mcp.Client | |
| llmMCPBridge *handlers.LLMMCPBridge | |
| llmRegistry *llm.ProviderRegistry | |
| cfg *config.Config | |
| messageHistory map[string][]Message | |
| historyLimit int | |
| discoveredTools map[string]mcp.ToolInfo | |
| participatedThreads map[string]bool | |
| threadMutex sync.RWMutex | |
| } | |
| // trackThreadParticipation marks a thread as participated by the bot | |
| func (c *Client) trackThreadParticipation(channelID, threadTS string) { | |
| if threadTS != "" { | |
| threadKey := fmt.Sprintf("%s:%s", channelID, threadTS) | |
| c.threadMutex.Lock() | |
| c.participatedThreads[threadKey] = true | |
| c.threadMutex.Unlock() | |
| c.logger.DebugKV("Tracked thread participation", "channel", channelID, "thread", threadTS) | |
| } | |
| } | |
| // hasParticipatedInThread checks if the bot has participated in a thread | |
| func (c *Client) hasParticipatedInThread(channelID, threadTS string) bool { | |
| if threadTS == "" { | |
| return false | |
| } | |
| threadKey := fmt.Sprintf("%s:%s", channelID, threadTS) | |
| c.threadMutex.RLock() | |
| participated := c.participatedThreads[threadKey] | |
| c.threadMutex.RUnlock() | |
| return participated | |
| } |
🤖 Prompt for AI Agents
In internal/slack/client.go around lines 326 to 342, the participatedThreads map
is accessed concurrently without synchronization, causing a potential race
condition. Introduce a mutex field in the Client struct to protect access to
participatedThreads. Update trackThreadParticipation and hasParticipatedInThread
methods to lock the mutex before reading or writing to the map and unlock it
afterward to ensure thread-safe access.
1 participant
Summary
Changes
Benefits
Testing
Dependencies
This PR depends on #93 (emoji reactions) being merged first.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements