fix(slack): preserve thread context and always send fallback text#6255
Open
Jacobinwwey wants to merge 11 commits intoAstrBotDevs:masterfrom
Open
fix(slack): preserve thread context and always send fallback text#6255Jacobinwwey wants to merge 11 commits intoAstrBotDevs:masterfrom
Jacobinwwey wants to merge 11 commits intoAstrBotDevs:masterfrom
Conversation
dosubot
bot
added
the
size:L
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在解决 Slack 适配器在处理线程消息时存在的关键问题，包括线程上下文丢失、回复消息位置不准确以及因 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
dosubot
bot
added
the
area:platform
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The channel/thread resolution logic is now duplicated between
SlackAdapter.send_by_sessionandSlackMessageEvent._resolve_*_target; consider extracting a shared helper (or reusingdecode_slack_session_idmore centrally) so the resolution rules stay consistent in one place. - Fallback strings like "消息", "[图片]" and "[文件:...]" are currently hardcoded in
_parse_slack_blocksand error fallbacks; if this adapter is used in non-Chinese environments, you may want to move these into constants or a localization/config layer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The channel/thread resolution logic is now duplicated between `SlackAdapter.send_by_session` and `SlackMessageEvent._resolve_*_target`; consider extracting a shared helper (or reusing `decode_slack_session_id` more centrally) so the resolution rules stay consistent in one place.
- Fallback strings like "消息", "[图片]" and "[文件:...]" are currently hardcoded in `_parse_slack_blocks` and error fallbacks; if this adapter is used in non-Chinese environments, you may want to move these into constants or a localization/config layer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
本次 PR 旨在修复 Slack 适配器中的线程上下文处理问题，并确保发送消息时总有 fallback 文本。整体改动方向正确，通过引入 session_codec 统一了线程会话 ID 的处理，并在消息发送和接收路径上都做了相应的适配，逻辑清晰。此外，补充的单元测试也很好地覆盖了核心场景。
我在代码中发现了一些可以改进的地方：
session_codec.py中的解码函数存在一个可能导致 channel ID 丢失的逻辑缺陷。slack_event.py的send方法中存在代码重复，可以进行重构以提升代码的可维护性。
具体的修改建议请见代码中的评论。
Comment on lines
+14
to
+17
| if THREAD_SESSION_MARKER in session_id: | ||
| channel_id, thread_ts = session_id.split(THREAD_SESSION_MARKER, 1) | ||
| if channel_id and thread_ts: | ||
| return channel_id, thread_ts |
Contributor
There was a problem hiding this comment.
当前对线程会话 ID 的解码逻辑存在一个缺陷：当 session_id 包含 __thread__ 标记但 channel_id 或 thread_ts 为空时，会错误地进入旧版 ID 格式的解析逻辑。例如，对于 "C123__thread__" 这样的 ID，会因为 thread_ts 为空而判断失败，然后 _ 的存在使其被 rsplit 错误处理，最终导致 channel_id 丢失。我们应该确保一旦检测到 __thread__ 标记，就只使用新的解码逻辑，即使 ID 格式不完全正确。
建议修改此逻辑，以更稳健地处理这些边缘情况，避免错误的逻辑穿透。
if THREAD_SESSION_MARKER in session_id:
channel_id, thread_ts = session_id.split(THREAD_SESSION_MARKER, 1)
return channel_id, thread_ts if channel_id and thread_ts else None
Comment on lines
+180
to
+199
| if self.get_group_id(): | ||
| # 发送到频道 | ||
| await self.web_client.chat_postMessage( | ||
| channel=self.get_group_id(), | ||
| text=text, | ||
| blocks=blocks or None, | ||
| ) | ||
| channel_id, thread_ts = self._resolve_group_target() | ||
| message_payload = { | ||
| "channel": channel_id, | ||
| "text": safe_text, | ||
| "blocks": blocks or None, | ||
| } | ||
| if thread_ts: | ||
| message_payload["thread_ts"] = thread_ts | ||
| await self.web_client.chat_postMessage(**message_payload) | ||
| else: | ||
| # 发送私信 | ||
| await self.web_client.chat_postMessage( | ||
| channel=self.get_sender_id(), | ||
| text=text, | ||
| blocks=blocks or None, | ||
| ) | ||
| channel_id, thread_ts = self._resolve_friend_target() | ||
| message_payload = { | ||
| "channel": channel_id, | ||
| "text": safe_text, | ||
| "blocks": blocks or None, | ||
| } | ||
| if thread_ts: | ||
| message_payload["thread_ts"] = thread_ts | ||
| await self.web_client.chat_postMessage(**message_payload) |
Contributor
There was a problem hiding this comment.
在 try 块中，处理群组消息和私信的逻辑几乎完全相同，唯一的区别在于获取 channel_id 和 thread_ts 的方法调用（_resolve_group_target vs _resolve_friend_target）。这部分代码可以被重构以消除重复，提高代码的可读性和可维护性。我们可以先根据消息类型确定目标，然后执行一次发送操作。
if self.get_group_id():
channel_id, thread_ts = self._resolve_group_target()
else:
channel_id, thread_ts = self._resolve_friend_target()
message_payload = {
"channel": channel_id,
"text": safe_text,
"blocks": blocks or None,
}
if thread_ts:
message_payload["thread_ts"] = thread_ts
await self.web_client.chat_postMessage(**message_payload)
Comment on lines
+212
to
+229
| if self.get_group_id(): | ||
| await self.web_client.chat_postMessage( | ||
| channel=self.get_group_id(), | ||
| text=fallback_text, | ||
| ) | ||
| channel_id, thread_ts = self._resolve_group_target() | ||
| fallback_payload = { | ||
| "channel": channel_id, | ||
| "text": fallback_text, | ||
| } | ||
| if thread_ts: | ||
| fallback_payload["thread_ts"] = thread_ts | ||
| await self.web_client.chat_postMessage(**fallback_payload) | ||
| else: | ||
| await self.web_client.chat_postMessage( | ||
| channel=self.get_sender_id(), | ||
| text=fallback_text, | ||
| ) | ||
| channel_id, thread_ts = self._resolve_friend_target() | ||
| fallback_payload = { | ||
| "channel": channel_id, | ||
| "text": fallback_text, | ||
| } | ||
| if thread_ts: | ||
| fallback_payload["thread_ts"] = thread_ts | ||
| await self.web_client.chat_postMessage(**fallback_payload) |
Contributor
There was a problem hiding this comment.
与 try 块中的情况类似，except 块中用于发送纯文本 fallback 消息的逻辑在群组和私信分支中也存在大量重复。这部分代码也应该进行重构，以遵循 DRY (Don't Repeat Yourself) 原则。
if self.get_group_id():
channel_id, thread_ts = self._resolve_group_target()
else:
channel_id, thread_ts = self._resolve_friend_target()
fallback_payload = {
"channel": channel_id,
"text": fallback_text,
}
if thread_ts:
fallback_payload["thread_ts"] = thread_ts
await self.web_client.chat_postMessage(**fallback_payload)
Author
Supplemental Notes (for reviewer follow-up)Thanks for the detailed feedback. I’ve applied a follow-up refactor in commit 2154dff on branch fix/slack-thread-context-and-text-fallback, with the following clarifications(this version(2154dff) of code is robust in my opinion):
Validation
|
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/slack/slack_adapter.py" line_range="97" />
<code_context>
- text=text,
- blocks=blocks if blocks else None,
- )
+ channel_id, thread_ts = resolve_slack_message_target(
+ session_id=session.session_id,
+ sender_id=session.session_id,
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a small adapter-local helper for mapping sessions to Slack targets and renaming the event-normalization helper to make thread handling behavior more explicit and self-contained within this adapter.
You can keep the new thread‑aware behavior but make the flow more explicit and localized, which should address most of the complexity concerns.
### 1. Hide `resolve_slack_message_target` behind a local, explicit helper
`send_by_session` currently passes `sender_id=session.session_id`, which is conceptually wrong and makes it unclear how the channel is derived.
Create a small adapter‑local helper that expresses intent in terms of the AstrBot session, and hides the encode/decode details:
```python
# inside SlackAdapter
@staticmethod
def _session_to_slack_target(session: MessageSesion) -> tuple[str, str | None]:
"""
Translate an AstrBot session into a Slack (channel_id, thread_ts).
Keeps all encoding/decoding logic behind this adapter-facing API.
"""
channel_id, thread_ts = resolve_slack_message_target(
session_id=session.session_id,
# If resolve_slack_message_target needs a real user id, pass it explicitly.
# If it doesn't, make sender_id optional in its signature and pass None here.
sender_id=getattr(session, "user_id", None),
)
return channel_id, thread_ts
```
Then simplify `send_by_session`:
```python
async def send_by_session(
self,
session: MessageSesion,
message_chain: MessageChain,
) -> None:
blocks, text = await SlackMessageEvent._parse_slack_blocks(
message_chain=message_chain,
web_client=self.web_client,
)
safe_text = text or SLACK_SAFE_TEXT_FALLBACK
try:
channel_id, thread_ts = self._session_to_slack_target(session)
message_payload: dict[str, Any] = {
"channel": channel_id,
"text": safe_text,
"blocks": blocks or None,
}
if thread_ts:
message_payload["thread_ts"] = thread_ts
await self.web_client.chat_postMessage(**message_payload)
except Exception as e:
logger.error(f"Slack 发送消息失败: {e}")
await super().send_by_session(session, message_chain)
```
This keeps the thread‑aware behavior and reuse of `resolve_slack_message_target`, but removes the confusing `sender_id=session.session_id` and centralizes how a `MessageSesion` becomes a Slack target into one function.
### 2. Make `_normalize_event_payload`’s behavior explicit and discoverable
Right now `_normalize_event_payload` changes the shape of the Slack event in a non‑obvious way before `convert_message` sees it. A small rename + docstring makes the contract much clearer:
```python
@staticmethod
def _unwrap_message_replied_event(event: dict) -> dict:
"""
Socket Mode may deliver thread updates as a 'message_replied' envelope.
For those events, merge the nested 'message' payload into the top-level
dict so that downstream logic can treat it like a normal 'message' with
correct user/text/thread_ts fields.
"""
if event.get("subtype") == "message_replied":
nested_message = event.get("message")
if isinstance(nested_message, dict):
merged = dict(event)
merged.update(nested_message)
if not merged.get("channel"):
merged["channel"] = event.get("channel", "")
return merged
return event
```
And in `convert_message`:
```python
async def convert_message(self, event: dict) -> AstrBotMessage:
event = self._unwrap_message_replied_event(event)
logger.debug(f"[slack] RawMessage {event}")
...
```
If you want to go one small step further, add a comment next to the `thread_ts` usage to tie it back to this unwrapping:
```python
thread_ts = str(event.get("thread_ts", "") or "")
# thread_ts may come from unwrapped 'message_replied' events.
```
These changes keep all functionality (including threads and message_replied handling) but make the control flow and data shapes easier to reason about from within this file, without removing the shared encode/decode utilities.
</issue_to_address>
### Comment 2
<location path="astrbot/core/platform/sources/slack/session_codec.py" line_range="12" />
<code_context>
+ return f"{channel_id}{THREAD_SESSION_MARKER}{thread_ts}"
+
+
+def decode_slack_session_id(session_id: str) -> tuple[str, str | None]:
+ if not session_id:
+ return "", None
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the decoding and target-resolution helpers into smaller, focused functions to separate responsibilities and make precedence and legacy handling clearer.
You can keep the behavior but make the responsibilities clearer and less coupled with a couple of small refactors.
### 1. Separate thread vs legacy handling in `decode_slack_session_id`
Right now, one function has to know about both the thread marker and historical `group_` formats. You can make that easier to read and change later by extracting narrowly scoped helpers and using them in `decode_slack_session_id`:
```python
THREAD_SESSION_MARKER = "__thread__"
LEGACY_GROUP_SESSION_PREFIX = "group_"
SLACK_SAFE_TEXT_FALLBACK = "message"
def _decode_thread_session_id(session_id: str) -> tuple[str, str | None] | None:
if THREAD_SESSION_MARKER not in session_id:
return None
channel_id, thread_ts = session_id.split(THREAD_SESSION_MARKER, 1)
return channel_id, thread_ts or None
def _decode_legacy_session_id(session_id: str) -> tuple[str, str | None] | None:
if not session_id.startswith(LEGACY_GROUP_SESSION_PREFIX):
return None
return session_id[len(LEGACY_GROUP_SESSION_PREFIX):], None
def decode_slack_session_id(session_id: str) -> tuple[str, str | None]:
if not session_id:
return "", None
# Thread format takes precedence.
decoded = _decode_thread_session_id(session_id)
if decoded is not None:
return decoded
# Backward compatibility for historical IDs like "group_<channel_id>".
decoded = _decode_legacy_session_id(session_id)
if decoded is not None:
return decoded
# Raw channel id.
return session_id, None
```
This keeps all existing behavior (thread format wins, then legacy, then raw id) but makes it obvious that historical compatibility is localized to `_decode_legacy_session_id`.
### 2. Split `resolve_slack_message_target` by use case
The current resolver has to understand all sources and precedence at once. You can reduce mental load by:
* isolating raw-event extraction, and
* exposing separate helpers for “event-based” vs “session-based” calls.
Internally you can keep the same precedence chain and keep `resolve_slack_message_target` as a thin wrapper to avoid breaking callers.
```python
def _extract_raw_target(raw_message: dict | None) -> tuple[str, str | None]:
if not isinstance(raw_message, dict):
return "", None
raw_channel_id = ""
raw_thread_ts = None
raw_channel = raw_message.get("channel")
if raw_channel:
raw_channel_id = str(raw_channel)
raw_thread = raw_message.get("thread_ts")
if raw_thread:
raw_thread_ts = str(raw_thread)
return raw_channel_id, raw_thread_ts
def resolve_target_from_event(
*,
session_id: str,
raw_message: dict,
group_id: str = "",
) -> tuple[str, str | None]:
parsed_channel_id, parsed_thread_ts = decode_slack_session_id(session_id)
raw_channel_id, raw_thread_ts = _extract_raw_target(raw_message)
channel_id = group_id or raw_channel_id or parsed_channel_id
return channel_id, raw_thread_ts or parsed_thread_ts
def resolve_target_from_session(
*,
session_id: str,
group_id: str = "",
fallback_channel_id: str = "",
) -> tuple[str, str | None]:
parsed_channel_id, parsed_thread_ts = decode_slack_session_id(session_id)
channel_id = group_id or parsed_channel_id or fallback_channel_id
return channel_id, parsed_thread_ts
```
Then keep your existing generic function as a wrapper, preserving the current source precedence:
```python
def resolve_slack_message_target(
session_id: str,
*,
raw_message: dict | None = None,
group_id: str = "",
sender_id: str = "",
) -> tuple[str, str | None]:
parsed_channel_id, parsed_thread_ts = decode_slack_session_id(session_id)
raw_channel_id, raw_thread_ts = _extract_raw_target(raw_message)
# Same precedence as before: group_id > raw_channel_id > parsed_channel_id > sender_id
channel_id = group_id or raw_channel_id or parsed_channel_id or sender_id
return channel_id, raw_thread_ts or parsed_thread_ts
```
Call sites can migrate over time to the more explicit `resolve_target_from_event` or `resolve_target_from_session`, which makes it obvious which data is expected and avoids accidental dependency on the fallback ordering.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dosubot
bot
added
size:XL
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_tool_result_dedupdictionary inToolLoopAgentRunnercan grow unbounded over long-lived processes with many unique tool/arg combinations; consider adding a simple eviction strategy (e.g., max size with LRU or periodic reset) to avoid potential memory growth. - Slack fallback strings (
SLACK_SAFE_TEXT_FALLBACK,SLACK_*_FALLBACK_*) are now hardcoded English literals; if multi-language output is expected elsewhere in the platform, consider routing these through the same localization/config mechanism to keep UX consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_tool_result_dedup` dictionary in `ToolLoopAgentRunner` can grow unbounded over long-lived processes with many unique tool/arg combinations; consider adding a simple eviction strategy (e.g., max size with LRU or periodic reset) to avoid potential memory growth.
- Slack fallback strings (`SLACK_SAFE_TEXT_FALLBACK`, `SLACK_*_FALLBACK_*`) are now hardcoded English literals; if multi-language output is expected elsewhere in the platform, consider routing these through the same localization/config mechanism to keep UX consistent.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Jacobinwwey
force-pushed
the
fix/slack-thread-context-and-text-fallback
branch
from
d0545e3 to
7af3441
Compare
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
SlackMessageEvent.send, the broadexcept Exceptionbranch builds a text-only fallback but doesn’t log the exception, which makes debugging Slack send failures harder; consider logging the exception (with channel/thread context) before sending the fallback. - There are now three slightly different target-resolution helpers (
resolve_slack_message_target,resolve_target_from_event,resolve_target_from_session) with subtly different precedence rules; consider consolidating them or clearly separating their responsibilities to avoid future divergence or inconsistent behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SlackMessageEvent.send`, the broad `except Exception` branch builds a text-only fallback but doesn’t log the exception, which makes debugging Slack send failures harder; consider logging the exception (with channel/thread context) before sending the fallback.
- There are now three slightly different target-resolution helpers (`resolve_slack_message_target`, `resolve_target_from_event`, `resolve_target_from_session`) with subtly different precedence rules; consider consolidating them or clearly separating their responsibilities to avoid future divergence or inconsistent behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
_session_to_slack_targetsignature inSlackAdapteris annotated withMessageSesioninstead ofMessageSession, which will break type checking and can confuse IDEs; update the annotation to the correct class name. - In
SlackMessageEvent.send, failures when callingchat_postMessagetrigger a logged exception and then a text-only retry, butSlackAdapter.send_by_sessionjust logs and returns; consider aligning these behaviors so session-based sends also attempt a safe text-only fallback for consistency. - Several user-facing strings were switched from Chinese to English (e.g., file upload messages and
file:label); if localization matters for existing users, consider either keeping the previous language or routing these through a configurable/i18n layer similar to thetext_fallbacksmechanism.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_session_to_slack_target` signature in `SlackAdapter` is annotated with `MessageSesion` instead of `MessageSession`, which will break type checking and can confuse IDEs; update the annotation to the correct class name.
- In `SlackMessageEvent.send`, failures when calling `chat_postMessage` trigger a logged exception and then a text-only retry, but `SlackAdapter.send_by_session` just logs and returns; consider aligning these behaviors so session-based sends also attempt a safe text-only fallback for consistency.
- Several user-facing strings were switched from Chinese to English (e.g., file upload messages and `file:` label); if localization matters for existing users, consider either keeping the previous language or routing these through a configurable/i18n layer similar to the `text_fallbacks` mechanism.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/slack/slack_adapter.py" line_range="104-113" />
<code_context>
+ )
+ channel_id, thread_ts = self._resolve_target()
try:
- if self.get_group_id():
- # 发送到频道
- await self.web_client.chat_postMessage(
- channel=self.get_group_id(),
- text=text,
- blocks=blocks or None,
- )
- else:
- # 发送私信
- await self.web_client.chat_postMessage(
- channel=self.get_sender_id(),
- text=text,
- blocks=blocks or None,
- )
+ message_payload = {
+ "channel": channel_id,
+ "text": safe_text,
+ "blocks": blocks or None,
+ }
+ if thread_ts:
+ message_payload["thread_ts"] = thread_ts
+ await self.web_client.chat_postMessage(**message_payload)
except Exception:
+ logger.exception(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align send_by_session error fallback behavior with SlackMessageEvent.send for better robustness.
Currently, `send_by_session` only logs on `chat_postMessage` failure and does not attempt a degraded text-only send. Please mirror the text-only retry behavior from `SlackMessageEvent.send` here (ideally via a shared helper to build the fallback) so session-based sends are equally resilient when Slack rejects blocks or attachments.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/slack/slack_adapter.py" line_range="89" />
<code_context>
+ platform_config.get("text_fallbacks"),
+ )
async def send_by_session(
self,
- session: MessageSesion,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the shared send-with-blocks-and-fallback logic into a reusable helper and calling `resolve_target_from_session` directly to reduce duplication and indirection between the adapter and event sender paths.
You can keep all the new behavior and still trim some complexity with a small refactor around sending + fallbacks and by reducing thin indirection.
### 1. Extract shared “send with blocks + fallback” helper
`SlackAdapter.send_by_session` is now very close in shape to what `SlackMessageEvent.send` likely does (blocks → safe_text → try send → build text fallback → retry). To avoid divergence, centralize this in a single helper that both paths use.
For example, in `slack_event.py` (or a small `slack_send_utils.py`):
```python
# slack_send_utils.py
from typing import Mapping, Any
from slack_sdk.web.async_client import AsyncWebClient
async def send_with_blocks_and_fallback(
*,
web_client: AsyncWebClient,
channel: str,
thread_ts: str | None,
message_chain: MessageChain,
text_fallbacks: Mapping[str, str],
) -> None:
blocks, text = await SlackMessageEvent._parse_slack_blocks(
message_chain=message_chain,
web_client=web_client,
text_fallbacks=text_fallbacks,
)
safe_text = text or text_fallbacks.get("safe_text", SLACK_SAFE_TEXT_FALLBACK)
message_payload: dict[str, Any] = {
"channel": channel,
"text": safe_text,
"blocks": blocks or None,
}
if thread_ts:
message_payload["thread_ts"] = thread_ts
try:
await web_client.chat_postMessage(**message_payload)
except Exception:
logger.exception(
"Slack send failed, retrying with text-only payload. "
"channel_id=%s thread_ts=%s",
channel,
thread_ts or "",
)
fallback_text = SlackMessageEvent._build_text_fallback_from_chain(
message_chain=message_chain,
text_fallbacks=text_fallbacks,
)
fallback_payload: dict[str, Any] = {
"channel": channel,
"text": fallback_text,
}
if thread_ts:
fallback_payload["thread_ts"] = thread_ts
try:
await web_client.chat_postMessage(**fallback_payload)
except Exception:
logger.exception(
"Slack send text-only fallback failed. "
"channel_id=%s thread_ts=%s",
channel,
thread_ts or "",
)
```
Then `SlackAdapter.send_by_session` becomes a thin adapter:
```python
# slack_adapter.py
from .slack_send_utils import send_with_blocks_and_fallback
async def send_by_session(
self,
session: MessageSession,
message_chain: MessageChain,
) -> None:
channel_id, thread_ts = resolve_target_from_session(
session_id=session.session_id
)
await send_with_blocks_and_fallback(
web_client=self.web_client,
channel=channel_id,
thread_ts=thread_ts,
message_chain=message_chain,
text_fallbacks=self.text_fallbacks,
)
await super().send_by_session(session, message_chain)
```
And in `SlackMessageEvent.send` you can directly reuse the same helper instead of re-implementing:
```python
async def send(self) -> None:
channel_id, thread_ts = resolve_target_from_session(
session_id=self.session_id
)
await send_with_blocks_and_fallback(
web_client=self.web_client,
channel=channel_id,
thread_ts=thread_ts,
message_chain=self.message_chain,
text_fallbacks=self.text_fallbacks,
)
```
This keeps all current behavior but removes duplicated send/fallback semantics and ensures future tweaks (additional fallbacks, payload fields, logging) are in one place.
### 2. Remove the thin `_session_to_slack_target` wrapper
Once you have the helper above, the `_session_to_slack_target` static method is basically just forwarding to `resolve_target_from_session`. You can simplify the call site and drop the extra layer:
```python
# before
channel_id, thread_ts = self._session_to_slack_target(session)
# after
channel_id, thread_ts = resolve_target_from_session(
session_id=session.session_id
)
```
And delete `_session_to_slack_target` entirely. This makes it clearer what mapping is happening without chasing an extra wrapper.
### 3. Make fallback ownership explicit
Since `SlackAdapter` already constructs `self.text_fallbacks` and passes it both to `SlackMessageEvent` and to the send helper, you can make the adapter the clear “owner” of fallback configuration and keep the event class logic focused on message semantics. The shared helper above reinforces that separation: adapter supplies config, helper + event code consume it.
If you want to go one step further, you can add a short docstring/comment where `self.text_fallbacks` is initialized to state that this is the canonical source of Slack text fallback rules, and that both adapter and events/handlers should depend on it via arguments rather than redefining their own behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
SlackMessageEvent._parse_slack_blocksyou callbuild_slack_text_fallbacksand then pass the result into_from_segment_to_slack_block, which callsbuild_slack_text_fallbacksagain; consider changing_from_segment_to_slack_blockto accept a pre-normalized fallbacks dict to avoid redundant normalization and potential confusion about expected input type. build_slack_text_fallbackscurrently ignores any override keys not present inSLACK_DEFAULT_TEXT_FALLBACKS; if you intend this helper to be extensible, consider either allowing extra keys through or adding a brief comment to clarify that only the predefined keys are honored.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SlackMessageEvent._parse_slack_blocks` you call `build_slack_text_fallbacks` and then pass the result into `_from_segment_to_slack_block`, which calls `build_slack_text_fallbacks` again; consider changing `_from_segment_to_slack_block` to accept a pre-normalized fallbacks dict to avoid redundant normalization and potential confusion about expected input type.
- `build_slack_text_fallbacks` currently ignores any override keys not present in `SLACK_DEFAULT_TEXT_FALLBACKS`; if you intend this helper to be extensible, consider either allowing extra keys through or adding a brief comment to clarify that only the predefined keys are honored.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/slack/session_codec.py" line_range="45" />
<code_context>
+ return session_id[len(LEGACY_GROUP_SESSION_PREFIX) :], None
+
+
+def decode_slack_session_id(session_id: str) -> tuple[str, str | None]:
+ if not session_id:
+ return "", None
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the thread and legacy prefix handling directly into `decode_slack_session_id` to make the session decoding logic more straightforward and self-contained.
You can simplify the session decoding logic by inlining the prefix/marker handling into a single function and removing the nullable-tuple helpers. This keeps behavior identical but reduces branching and indirection:
```python
def decode_slack_session_id(session_id: str) -> tuple[str, str | None]:
"""Decode a Slack session id into (channel_id, thread_ts|None)."""
if not session_id:
return "", None
# thread-aware session id: "<channel>__thread__<ts>"
if THREAD_SESSION_MARKER in session_id:
channel_id, thread_ts = session_id.split(THREAD_SESSION_MARKER, 1)
return channel_id, thread_ts or None
# legacy group session id: "group_<channel>"
if session_id.startswith(LEGACY_GROUP_SESSION_PREFIX):
return session_id[len(LEGACY_GROUP_SESSION_PREFIX) :], None
# plain channel id
return session_id, None
```
Then you can safely drop `_decode_thread_session_id` and `_decode_legacy_session_id`, and the rest of the module (`_resolve_target_with_precedence`, `resolve_*`) remains unchanged. This keeps all existing behaviors (thread-aware IDs, legacy group IDs, and plain channel IDs) while making the decoding path easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dosubot
bot
removed
the
size:XL
dosubot
bot
added
the
size:XXL
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The fallback config is being repeatedly normalized (e.g.,
build_slack_text_fallbacksis called insend_with_blocks_and_fallback, then again insideparse_slack_blocksandbuild_text_fallback_from_chain); consider passing a pre-normalized dict through these layers to avoid redundant work and make the call graph clearer. - In
SlackMessageEvent._from_segment_to_slack_blockyou recomputefallbacks = build_slack_text_fallbacks(text_fallbacks)for each segment; it would be more efficient to thread a precomputedfallbacksdict from the caller (similar tofrom_segment_to_slack_block) instead of rebuilding it per call.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The fallback config is being repeatedly normalized (e.g., `build_slack_text_fallbacks` is called in `send_with_blocks_and_fallback`, then again inside `parse_slack_blocks` and `build_text_fallback_from_chain`); consider passing a pre-normalized dict through these layers to avoid redundant work and make the call graph clearer.
- In `SlackMessageEvent._from_segment_to_slack_block` you recompute `fallbacks = build_slack_text_fallbacks(text_fallbacks)` for each segment; it would be more efficient to thread a precomputed `fallbacks` dict from the caller (similar to `from_segment_to_slack_block`) instead of rebuilding it per call.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
send_with_blocks_and_fallbackthe dualtext_fallbacksandfallbacksparameters make the fallback resolution path harder to follow and can recompute defaults unnecessarily; consider simplifying to a single, already-normalized fallbacks argument and letting callers pass the prebuilt dict frombuild_slack_text_fallbacks. - The target resolution helpers can return an empty
channel_id(e.g., when both session and raw payload lack a channel), which will then be passed directly intochat_postMessage; consider adding an explicit guard/log-and-return path whenchannelis empty to avoid noisy runtime errors from the Slack API.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `send_with_blocks_and_fallback` the dual `text_fallbacks` and `fallbacks` parameters make the fallback resolution path harder to follow and can recompute defaults unnecessarily; consider simplifying to a single, already-normalized fallbacks argument and letting callers pass the prebuilt dict from `build_slack_text_fallbacks`.
- The target resolution helpers can return an empty `channel_id` (e.g., when both session and raw payload lack a channel), which will then be passed directly into `chat_postMessage`; consider adding an explicit guard/log-and-return path when `channel` is empty to avoid noisy runtime errors from the Slack API.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/slack/slack_send_utils.py" line_range="143" />
<code_context>
+ {"type": "section", "text": {"type": "mrkdwn", "text": text_content}},
+ )
+
+ fallback_text = "".join(fallback_parts).strip() or resolved_fallbacks["safe_text"]
+ return blocks, fallback_text if blocks else text_content
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The computed `fallback_text` is ignored when no blocks are produced, which can drop useful placeholders.
`fallback_text` is only returned when `blocks` is non-empty:
```python
fallback_text = "".join(fallback_parts).strip() or resolved_fallbacks["safe_text"]
return blocks, fallback_text if blocks else text_content
```
If all rich segments fail to produce blocks but still add to `fallback_parts`, `blocks` will be empty and `text_content` may still be `""`. The caller then sees `text == ""` and falls back to the generic `safe_text`, dropping the more informative placeholders from `fallback_parts`.
To preserve those placeholders, always prefer `fallback_text` when available, e.g.:
```python
fallback_text = "".join(fallback_parts).strip() or resolved_fallbacks["safe_text"]
return blocks, fallback_text
```
(or key off `fallback_parts` rather than `blocks`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Mar 16, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/slack/session_codec.py" line_range="52" />
<code_context>
+ return session_id, None
+
+
+def _extract_raw_target(raw_message: dict | None) -> tuple[str, str | None]:
+ if not isinstance(raw_message, dict):
+ return "", None
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the raw-target extraction and precedence logic into a single resolver and using thin wrappers to reduce indirection and public surface area.
You can simplify this without changing behavior by collapsing the small state machine into a single resolver and, optionally, reducing the number of public entry points.
### 1. Inline `_extract_raw_target` and `_resolve_target_with_precedence`
The two-step `_extract_raw_target` → `_resolve_target_with_precedence` chain can be expressed more directly in `resolve_slack_message_target`, which already defines the full API surface.
```python
def resolve_slack_message_target(
session_id: str,
*,
raw_message: dict | None = None,
group_id: str = "",
sender_id: str = "",
) -> tuple[str, str | None]:
"""Backward-compatible resolver shared by legacy and new Slack call sites.
Precedence for channel: group_id > raw_message.channel > parsed(session_id) > sender_id
Precedence for thread: raw_message.thread_ts > parsed(session_id)
"""
# Decode session
parsed_channel_id, parsed_thread_ts = decode_slack_session_id(session_id)
# Extract from raw message, if present
raw_channel_id = ""
raw_thread_ts = None
if isinstance(raw_message, dict):
raw_channel = raw_message.get("channel")
if raw_channel not in (None, ""):
raw_channel_id = str(raw_channel)
raw_thread = raw_message.get("thread_ts")
if raw_thread not in (None, ""):
raw_thread_ts = str(raw_thread)
channel_id = group_id or raw_channel_id or parsed_channel_id or sender_id
thread_ts = raw_thread_ts or parsed_thread_ts
return channel_id, thread_ts
```
With this in place, you can delete `_extract_raw_target` and `_resolve_target_with_precedence` entirely, while keeping the same precedence behavior.
### 2. Keep “convenience” wrappers trivially thin (or mark them internal)
If `resolve_target_from_event` / `resolve_target_from_session` are useful for readability at call sites, have them delegate to the single resolver instead of a separate internal helper:
```python
def resolve_target_from_event(
*,
session_id: str,
raw_message: dict,
group_id: str = "",
) -> tuple[str, str | None]:
"""Resolve target for received Slack events (uses event raw payload)."""
return resolve_slack_message_target(
session_id=session_id,
raw_message=raw_message,
group_id=group_id,
)
def resolve_target_from_session(
*,
session_id: str,
group_id: str = "",
fallback_channel_id: str = "",
) -> tuple[str, str | None]:
"""Resolve target when only session metadata is available (no raw event)."""
return resolve_slack_message_target(
session_id=session_id,
group_id=group_id,
sender_id=fallback_channel_id,
)
```
If they’re not widely used, another option is to make only `resolve_slack_message_target` public and treat these as private/module-local helpers (or inline their behavior at call sites). This reduces the number of “official” ways to resolve Slack targets, making the precedence rules easier to find and reason about.
</issue_to_address>Hi @Jacobinwwey! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/slack/slack_send_utils.py" line_range="221" />
<code_context>
+ thread_ts or "",
+ )
+
+ fallback_text = build_text_fallback(message_chain, resolved_fallbacks)
+ fallback_payload: dict[str, Any] = {
+ "channel": channel,
</code_context>
<issue_to_address>
**suggestion:** Guard against an empty fallback text when building the text-only retry payload.
This assumes `build_text_fallback` always returns a non-empty string. That’s true for `build_text_fallback_from_chain` due to `safe_text`, but a custom implementation could return `""` and degrade the Slack error message. Consider mirroring the earlier `safe_text` guard here (e.g. `fallback_text or resolved_fallbacks["safe_text"]`) to stay robust to alternative implementations.
```suggestion
# Ensure we always have a non-empty fallback text, even if a custom
# build_text_fallback implementation returns an empty string.
fallback_text = build_text_fallback(message_chain, resolved_fallbacks) or resolved_fallbacks["safe_text"]
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
Author
|
@Fridemn @Raven95676 [Passed the review by Sourcery] |
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
2 participants
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.
Motivation / 动机
Slack 适配器在线程场景存在两个实际问题：
thread_ts，导致部分回复落回主频道而非线程。chat.postMessage在 blocks-only 场景可能出现空text，触发 Slack SDK warning（The top-level text argument is missing...）。本 PR 统一修复上述问题，并补充回归测试。
Modifications / 改动点
新增 Slack 会话编解码工具：
astrbot/core/platform/sources/slack/session_codec.py修复 Slack 消息接收与会话构建：
astrbot/core/platform/sources/slack/slack_adapter.pymessage_replied事件做归一化处理，确保user/text/thread_ts/channel可被正确提取。thread_ts时（群聊或私聊），会话 ID 使用线程维度，确保线程上下文隔离。修复 Slack 消息发送路径：
astrbot/core/platform/sources/slack/slack_adapter.pyastrbot/core/platform/sources/slack/slack_event.pythread_ts，保证在线程内回复。textfallback（非空兜底）以满足 Slackchat.postMessage最佳实践，消除运行期 warning。新增单元测试：
tests/unit/test_slack_thread_session.pymessage_replied归一化、线程发送与 fallback 文本行为。This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
本地验证命令与结果：
uv run ruff check astrbot/core/platform/sources/slack/slack_adapter.py astrbot/core/platform/sources/slack/slack_event.py astrbot/core/platform/sources/slack/session_codec.py tests/unit/test_slack_thread_session.pyAll checks passed!uv run pytest -q tests/unit/test_slack_thread_session.py8 passed, 3 warnings in 1.27s运行期验证：
chat.postMessage缺失顶层text的 warning（新重启后日志窗口内验证）。Checklist / 检查清单
Summary by Sourcery
Improve Slack adapter to keep replies within threads, preserve thread-based session context, and ensure all outgoing messages include a safe text fallback alongside blocks.
Bug Fixes:
Enhancements:
Tests: