Add Slack channel support#7
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactoring and expansion of the AI agent codebase, restructuring the project into dedicated core, infra, and cli modules. It adds Discord and Slack channels alongside Telegram, implements a SQLite-backed scheduled tasks runner, and introduces several new tools (calculator, HackerNews, web search, etc.). The code review highlights critical bugs, including an infinite loop risk in the agent loop when finish_reason is not "stop", and a delivery routing failure for scheduled tasks after restarts. Additionally, several performance and stability concerns were raised, such as synchronous blocking calls freezing the asyncio event loop, potential startup crashes from unhandled environment variable parsing, and dead code in the newly added channels.
| else: | ||
| await self._on_response(assistant_message.content) | ||
| if finish_reason == "stop": | ||
| messages.append(self._serialize_assistant_msg(assistant_message)) | ||
| break |
There was a problem hiding this comment.
There is a critical infinite loop bug when finish_reason is not "stop" (e.g., "length"). Currently, if finish_reason is not "stop", the assistant's partial response is not appended to messages, and the loop continues to the next iteration. This causes the next API call to send the exact same messages, resulting in the LLM generating the exact same response and causing an infinite loop of identical API calls until max_iterations is reached.\n\nTo fix this, always append the assistant's response to messages before checking if the loop should terminate.
else:\n await self._on_response(assistant_message.content)\n messages.append(self._serialize_assistant_msg(assistant_message))\n if finish_reason == "stop":\n break| if mq and channel: | ||
| try: | ||
| from ..channels.message import OutgoingMessage | ||
| await mq.outgoing_msg(OutgoingMessage(content=output, channel=channel, metadata=channel.default_metadata)) |
There was a problem hiding this comment.
There is a critical routing and delivery bug for scheduled tasks. Relying on channel.default_metadata (which uses the volatile _last_channel_id) has two major issues:\n1. Delivery Failure after Restart: If the bot restarts, _last_channel_id is None, so default_metadata is {}, causing delivery to fail completely for Slack and Telegram.\n2. Cross-Channel Leakage: Since _last_channel_id is overwritten by the most recent message across any channel/chat, a scheduled task created in Channel A will be delivered to Channel B if Channel B was active more recently.\n\nTo fix this, the tasks table should store the destination metadata (like chat_id or channel_id) when the task is created, and use that stored metadata during delivery instead of relying on the channel's volatile global state.
| if not await self._check_permission(tool_name, tool_args): | ||
| return "User denied permission to run this tool. Ask for permission to run the tool again if you want to try running it." | ||
| await self._on_tool_start(tool_name, tool_args) | ||
| return run_tool(tool_name=tool_name, tool_args=tool_args) |
There was a problem hiding this comment.
Calling run_tool synchronously blocks the entire asyncio event loop. Since this is a multi-channel background server running Telegram, Discord, and Slack concurrently, any blocking tool call (like bash running a long command, or web_fetch taking several seconds) will freeze the entire server for all users.\n\nTo prevent this, tools should ideally be run in a thread pool (e.g., using asyncio.to_thread). However, note that run_tool currently uses os.chdir to manage the working directory, which is not thread-safe. To make it thread-safe, you should avoid process-wide directory changes and instead pass absolute paths or the workspace directory directly to the tools.
| def call(command: str, argument1: int, argument2: int = None) -> str: | ||
| log.info(f"calculator, command: {command}") | ||
|
|
||
| try: | ||
| if command == "add": |
There was a problem hiding this comment.
The argument2 parameter is optional in the tool spec, but its type is annotated as int instead of int | None. Furthermore, if the LLM calls a two-argument command (like add, subtract, multiply, divide, exponentiate) and omits argument2, it will be passed as None, causing a TypeError when performing the operation.\n\nYou should update the type annotation and add a check to ensure argument2 is not None for these commands.
def call(command: str, argument1: int, argument2: int | None = None) -> str:\n log.info(f"calculator, command: {command}")\n\n try:\n if command in ["add", "subtract", "multiply", "divide", "exponentiate"] and argument2 is None:\n return f"Error: command '{command}' requires a second argument"\n\n if command == "add":| chat = await self.client.chat.completions.create( | ||
| model = runtime.get("model", "deepseek/deepseek-v4-flash"), | ||
| messages = messages, | ||
| tools = tool_specs, | ||
| ) |
There was a problem hiding this comment.
Passing an empty list to the tools parameter in chat.completions.create can cause API errors with some LLM providers or OpenRouter. It is safer to only include the tools parameter if tool_specs is non-empty.
kwargs = {\n "model": runtime.get("model", "deepseek/deepseek-v4-flash"),\n "messages": messages,\n }\n if tool_specs:\n kwargs["tools"] = tool_specs\n chat = await self.client.chat.completions.create(**kwargs)| async def error_handler(self, update, context) -> None: | ||
| log.error(f"Discord error: {context}") |
There was a problem hiding this comment.
The error_handler method is dead code. It has a signature copied from python-telegram-bot (update, context) which is incorrect for discord.py, and it is never registered. It should be removed.
| finally: | ||
| duration = (datetime.now() - start).total_seconds() | ||
| self.save_output(name=name, prompt=prompt, output=output, | ||
| status=status, duration_secs=duration) | ||
| self._after_run(task=task, now=datetime.now()) |
There was a problem hiding this comment.
Raising an exception inside a finally block (such as a SQLite OperationalError like "database is locked" inside _after_run) will discard and mask any original exception that occurred in the try block. It is safer to wrap the database operations inside finally in their own try...except blocks to prevent masking.
| if v := os.environ.get("TELEGRAM_BOT_TOKEN"): | ||
| _config.setdefault("telegram", {})["BOT_TOKEN"] = v | ||
| if v := os.environ.get("TELEGRAM_ALLOW_FROM"): | ||
| _config.setdefault("telegram", {})["ALLOW_FROM"] = [int(x.strip()) for x in v.split(",") if x.strip()] |
There was a problem hiding this comment.
If the TELEGRAM_ALLOW_FROM environment variable contains any non-integer values (e.g., due to a typo like "123, abc"), calling int(x.strip()) will raise a ValueError on startup, crashing the entire bot. Consider filtering out non-digit values or catching the exception to fail gracefully.
| _config.setdefault("telegram", {})["ALLOW_FROM"] = [int(x.strip()) for x in v.split(",") if x.strip()] | |
| _config.setdefault("telegram", {})["ALLOW_FROM"] = [int(x.strip()) for x in v.split(",") if x.strip().lstrip('-').isdigit()] |
| if v := os.environ.get("DISCORD_BOT_TOKEN"): | ||
| _config.setdefault("discord", {})["TOKEN"] = v | ||
| if v := os.environ.get("DISCORD_ALLOW_FROM"): | ||
| _config.setdefault("discord", {})["ALLOW_FROM"] = [int(x.strip()) for x in v.split(",") if x.strip()] |
There was a problem hiding this comment.
If the DISCORD_ALLOW_FROM environment variable contains any non-integer values, calling int(x.strip()) will raise a ValueError on startup, crashing the entire bot. Consider filtering out non-digit values or catching the exception to fail gracefully.
| _config.setdefault("discord", {})["ALLOW_FROM"] = [int(x.strip()) for x in v.split(",") if x.strip()] | |
| _config.setdefault("discord", {})["ALLOW_FROM"] = [int(x.strip()) for x in v.split(",") if x.strip().lstrip('-').isdigit()] |
| def error_handler(self, update, context) -> None: | ||
| log.error(f"Slack error: {context}") |
There was a problem hiding this comment.
The error_handler method is dead code. It has a signature copied from python-telegram-bot (update, context) which is incorrect for Slack Bolt, and it is never registered with self.app. It should be removed to keep the codebase clean.
There was a problem hiding this comment.
Pull request overview
Adds a Slack channel implementation alongside the existing Telegram and Discord channels, using slack-bolt in Socket Mode so no public webhook is required. The new channel follows the same message-queue / BackgroundAgent wiring as the others.
Changes:
- New
SlackChannel(app/channels/slack.py) with message handling,/whoami//stopinterception, send chunking, and async Socket Mode polling; newChannelType.SLACKenum value. bg_server.pywires Slack alongside Telegram/Discord (ownMessageQueue,BackgroundAgent, and coroutines), andconfig.pyaddsSLACK_BOT_TOKEN/SLACK_APP_TOKEN/SLACK_ALLOW_FROMenv handling plus a[slack]section in the default config.- Adds
slack-bolt>=1.18.0topyproject.toml/uv.lockand 21 unit tests intests/test_slack_channel.py.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/channels/slack.py | New SlackChannel mirroring Discord/Telegram patterns. |
| app/channels/channel.py | Adds ChannelType.SLACK. |
| app/bg_server.py | Wires the Slack channel/agent/queue into startup. |
| app/config.py | Adds Slack env-var parsing and default [slack] config section. |
| pyproject.toml | Adds slack-bolt>=1.18.0 dependency. |
| uv.lock | Locks slack-bolt 1.28.0 and slack-sdk 3.42.0. |
| tests/test_slack_channel.py | Unit tests covering init, message handling, commands, send chunking, and error paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| MAX_SLACK_LENGTH = 3000 |
Implements SlackChannel following the same conventions as the existing Discord and Telegram channels. Wires it into bg_server and config (env vars SLACK_BOT_TOKEN, SLACK_APP_TOKEN, SLACK_ALLOW_FROM). https://claude.ai/code/session_01Fa64TcstSLyfugG93cjYR2
- Remove dead _bot_user_id field - Only filter system subtypes (message_changed/deleted, join/leave, etc.) instead of all subtypes — allows file_share, me_message, thread_broadcast - Register a slack_bolt-compatible error handler via app.error() - Update README: add Slack to feature list, config, Docker section, env table - Update Dockerfile: add SLACK_ALLOW_FROM env default and usage comments - Remove Slack from roadmap (shipped) https://claude.ai/code/session_01Fa64TcstSLyfugG93cjYR2
Rikul
force-pushed
the
claude/slack-channel-support-ipLnV
branch
from
fe367d3 to
ce9039b
Compare
- Fix exc_info=error -> exc_info=True in _slack_error_handler (logging API) - Guard against events with empty/missing channel_id before enqueuing - Raise MAX_SLACK_LENGTH 3000 -> 3800 (plain text= field allows ~40k chars) - Remove unused MagicMock import from test_slack_channel.py - Add test for empty-channel guard - Add Slack env var tests to test_config.py (BOT_TOKEN, APP_TOKEN, ALLOW_FROM) - Add Slack branch tests to test_bg_server.py (happy path, missing tokens, gather coros) https://claude.ai/code/session_01Fa64TcstSLyfugG93cjYR2
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef23be7ea5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Empty ALLOW_FROM now means no one is permitted, consistent with the principle of least privilege. Previously [] was treated as "no filter" (allow everyone); now it is treated as an empty allowlist (allow nobody). Update config comments, README, and all affected tests accordingly. https://claude.ai/code/session_01Fa64TcstSLyfugG93cjYR2
…am auth gap
- Route Slack slash commands through app.command() handler (not just event("message")),
matching how Slack actually delivers /commands
- Update _last_channel_id before the command branch in _handle_message so it is
always set even when the message triggers a whoami/stop inline response
- Add allow_from auth check to Telegram command_handler, which previously had no
authorization guard and allowed any user to invoke slash commands
- Add tests for all three cases
https://claude.ai/code/session_01Fa64TcstSLyfugG93cjYR2
…ompts - CLAUDE.md described run_tool() as synchronous and claimed MCP tool routing uses rpartition; both are wrong (run_tool is async, routing uses partition) - helper_agent tool logged the full user prompt at INFO level, which can leak sensitive data (tokens, credentials) into application logs; log only the prompt length instead
…support-ipLnV # Conflicts: # CLAUDE.md # README.md # app/tools/helper_agent.py # pyproject.toml
| if v := os.environ.get("SLACK_BOT_TOKEN"): | ||
| _config.setdefault("slack", {})["BOT_TOKEN"] = v | ||
| if v := os.environ.get("SLACK_APP_TOKEN"): | ||
| _config.setdefault("slack", {})["APP_TOKEN"] = v | ||
| if v := os.environ.get("SLACK_ALLOW_FROM"): | ||
| _config.setdefault("slack", {})["ALLOW_FROM"] = [x.strip() for x in v.split(",") if x.strip()] |
| if slack_channel: | ||
| channels["slack"] = slack_channel | ||
| mqs["slack"] = slack_mq |
| ALLOW_FROM = [] # List of allowed Telegram user IDs (integers). | ||
|
|
||
| [discord] | ||
| TOKEN = "" | ||
| ALLOW_FROM = [] # List of allowed Discord user IDs (integers). Empty means allow all. | ||
| ALLOW_FROM = [] # List of allowed Discord user IDs (integers). Empty = deny all. |
| [discord] | ||
| TOKEN = "your-discord-bot-token" | ||
| ALLOW_FROM = [] # restrict by user ID; empty = allow all | ||
| ALLOW_FROM = [] # restrict by user ID | ||
|
|
| [slack] | ||
| BOT_TOKEN = "xoxb-..." # bot token from Slack app settings | ||
| APP_TOKEN = "xapp-..." # app-level token with connections:write scope | ||
| ALLOW_FROM = [] # restrict by Slack user ID |
Summary
SlackChannelinapp/channels/slack.pyusingslack-boltwith Socket Mode (no public webhook needed)bg_server.pyalongside existing Discord/Telegram channelsSLACK_BOT_TOKEN,SLACK_APP_TOKEN,SLACK_ALLOW_FROMenv var support inconfig.pyConfiguration
Set the following env vars (or add a
[slack]section to~/.crafterscode/config.toml):SLACK_BOT_TOKEN—xoxb-...bot OAuth tokenSLACK_APP_TOKEN—xapp-...app-level token (requiresconnections:writescope for Socket Mode)SLACK_ALLOW_FROM— comma-separated Slack user IDs to restrict access (empty = deny all; must be non-empty to allow any user)This matches the existing Telegram/Discord
ALLOW_FROMbehavior, which also denies all users when empty.Test plan
tests/test_slack_channel.pyall pass/whoamireturns your Slack user ID/stophalts the agent loop/status) are forwarded toBackgroundAgentGenerated by Claude Code