fix(lark): Defer card creation and renew on tool call break#6743
fix(lark): Defer card creation and renew on tool call break#6743RC-CHN merged 4 commits intoAstrBotDevs:masterfrom
Conversation
…break - Defer CardKit streaming card creation until the first text token arrives, preventing an empty card from rendering before content. - Handle `type="break"` signal in send_streaming: close the current card and lazily create a new one for post-tool-call text, so the new card appears below the tool status message in correct order. - Only emit "break" signal when show_tool_use is enabled; when tool output is hidden, the AI response continues on the same card.
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! This pull request addresses two user experience issues in Lark's streaming output: the rendering of blank cards before the first LLM token arrives and message misalignment when tool calls interrupt the LLM's response. By deferring the creation of streaming cards and implementing a mechanism to renew cards upon a tool call break, the changes ensure a smoother and more coherent user experience, especially when interacting with AI agents that utilize tools. Highlights
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. Footnotes
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the fallback path (
_consume_rest_and_fallback), after youreturnfromsend_streamingthefinallyblock still runs and hits thecard_id is Nonebranch, causingMetric.uploadand_has_send_operto be executed twice for the same request; consider adding a flag to distinguish the fallback path or skipping the metric upload when a non-streaming send has already been performed. - When handling
type == "break", you awaitsender_taskinside theasync forloop and then later also await it again in the outerfinally(if it wasn’t reset in some edge path); it may be safer to explicitly setsender_task = Nonein all early-return/error branches around card closing to avoid any possibility of double-awaiting a finished task.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the fallback path (`_consume_rest_and_fallback`), after you `return` from `send_streaming` the `finally` block still runs and hits the `card_id is None` branch, causing `Metric.upload` and `_has_send_oper` to be executed twice for the same request; consider adding a flag to distinguish the fallback path or skipping the metric upload when a non-streaming send has already been performed.
- When handling `type == "break"`, you await `sender_task` inside the `async for` loop and then later also await it again in the outer `finally` (if it wasn’t reset in some edge path); it may be safer to explicitly set `sender_task = None` in all early-return/error branches around card closing to avoid any possibility of double-awaiting a finished task.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/lark/lark_event.py" line_range="743-751" />
<code_context>
async def send_streaming(self, generator, use_fallback: bool = False):
</code_context>
<issue_to_address>
**question (bug_risk):** `use_fallback` argument is no longer respected in the streaming path.
This used to route failures through `_fallback_send_streaming(generator, use_fallback)`, so callers could control behavior via `use_fallback`. Now the argument is ignored and all failures go through `_consume_rest_and_fallback`. If `use_fallback` is still part of the public contract, please either restore conditional handling based on it or remove/rename the parameter to avoid a silent behavior change.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request refactors the Lark streaming card implementation to improve user experience by deferring card creation until the first text token arrives and correctly handling message breaks during tool calls. The changes are well-structured and include robust error handling and fallbacks. My review includes one suggestion to improve maintainability by reducing code duplication.
| # Flush final text to the current card | ||
| if delta and delta != last_sent: | ||
| sequence += 1 | ||
| await self._update_streaming_text( | ||
| card_id, delta, sequence | ||
| ) | ||
| sequence += 1 | ||
| await self._close_streaming_mode(card_id, sequence) |
There was a problem hiding this comment.
This block of code for flushing the final text and closing the card is also present at the end of the send_streaming function (lines 866-872). Duplicating this logic can make future maintenance more difficult, as any changes would need to be applied in both places. Consider extracting this logic into a private helper method to improve code reuse and maintainability.
fix(lark): defer streaming card creation and renew card on tool call break
Motivation / 动机
修复飞书流式输出的两个体验问题:
Modifications / 改动点
astrbot/core/platform/sources/lark/lark_event.py_create_streaming_card()和_send_card_message()延迟到第一个Plain文本组件到来时才执行,避免空卡片渲染。break分段信号:收到type="break"时,刷新并关闭当前卡片,重置状态。下一个文本 token 到来时通过 lazy-init 自动创建新卡片,确保新卡片出现在工具状态消息下方,符合用户阅读顺序。astrbot/core/astr_agent_run_util.py条件化
break信号:将break的发送条件从if agent_runner.streaming改为if agent_runner.streaming and show_tool_use。当用户配置不显示工具输出时,不发送break,AI 回复在同一张卡片上连续更新,避免不必要的卡片分裂。补充中文注释:详细说明
break信号的含义、用途和条件约束。This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
验证步骤:
Card #1中正确关闭Card #1下方Card #2中展示(位于工具消息下方)Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了"验证步骤"和"运行截图"。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。