Conversation
…ndling of stop requests
dosubot
bot
added
the
size:L
Summary of ChangesHello @Soulter, 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 significantly enhances user control over active agent sessions by implementing a robust stop mechanism across both the backend and frontend. It allows users to interrupt ongoing agent tasks via a new command and a dedicated UI button, with the system designed to gracefully handle these interruptions and preserve any partial outputs. Additionally, it addresses a critical bug related to file attachment persistence, ensuring a more reliable chat experience. 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
feature:chatui
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![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 valuable feature to stop active agent sessions, along with several related bug fixes and enhancements. The implementation for both backend and frontend is well-structured, allowing for user-initiated interruption of chat generation while preserving partial results. The fix for file segment persistence by unifying attachment paths and handling legacy paths is a good improvement. I've identified one area for refactoring in astrbot/core/astr_agent_run_util.py where duplicated code for task cancellation can be simplified using a try...finally block, which would improve maintainability. Overall, this is a solid contribution.
| if resp.type == "aborted": | ||
| if not stop_watcher.done(): | ||
| stop_watcher.cancel() | ||
| try: | ||
| await stop_watcher | ||
| except asyncio.CancelledError: | ||
| pass | ||
| astr_event.set_extra("agent_user_aborted", True) | ||
| astr_event.set_extra("agent_stop_requested", False) | ||
| return |
There was a problem hiding this comment.
The logic to cancel the stop_watcher task is duplicated in three places (here, after the loop on lines 145-150, and in the except block on lines 164-169). This increases code complexity and the risk of bugs if one of the locations is missed during future changes.
Consider refactoring this using a try...finally block to centralize the cleanup logic. This ensures stop_watcher is always cancelled, regardless of how the try block is exited (e.g., via return, break, or an exception).
Example:
stop_watcher = asyncio.create_task(...)
try:
# Main agent loop logic
async for resp in agent_runner.step():
# ...
if resp.type == "aborted":
# ... set extras
return
# ...
# ...
except Exception as e:
# ... error handling
return
finally:
if not stop_watcher.done():
stop_watcher.cancel()
try:
await stop_watcher
except asyncio.CancelledError:
pass![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些高层次的反馈：
- 围绕
run_agent和ToolLoopAgentRunner的停止逻辑有点变得复杂了（行内的_should_stop_agent检查，加上_watch_agent_stop_signal任务，再加上对resp.type == "aborted"的处理）；建议把这些合并到一条单一且文档清晰的停止路径中，以减少竞态条件或重复取消行为的可能性。 - 在
_save_to_history中，user_aborted分支有多个特殊处理，还有一段被注释掉的 message 追加代码；最好要么删除这些注释掉的代码并简化 role/completion 的处理，要么清晰定义一次中止 run 时的历史记录表示形式，以便更容易理解这段逻辑。
给 AI Agents 的提示
Please address the comments from this code review:
## Overall Comments
- The stop logic around `run_agent` and `ToolLoopAgentRunner` has become a bit convoluted (inline `_should_stop_agent` checks plus the `_watch_agent_stop_signal` task plus `resp.type == "aborted"` handling); consider consolidating this into a single, clearly documented stopping path to reduce the chance of races or duplicated cancellation behavior.
- In `_save_to_history`, the `user_aborted` path has several special cases and a commented‑out message append block; it would be good to either remove the commented code and simplify the role/completion handling, or explicitly define the intended history representation for aborted runs so that this logic is easier to reason about.
## Individual Comments
### Comment 1
<location> `tests/test_tool_loop_agent_runner.py:419-428` </location>
<code_context>
+@pytest.mark.asyncio
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen assertions to verify the persisted partial message content, not only the role.
Right now the test only validates `final_resp.completion_text == "partial "` and that the last `run_context` message has role `"assistant"`. To better validate persistence, also assert that the last message’s content (e.g., its `TextPart`) includes the expected text chunk `"partial "`. This ensures we catch regressions where `final_llm_resp` is correct but the `run_context` messages are not updated.
Suggested implementation:
```python
assert final_resp.completion_text == "partial "
messages = runner.run_context.messages
last_msg = messages[-1]
# Ensure the last message is from the assistant
assert last_msg.role == "assistant"
# Ensure the partial text was actually persisted on the last message content
# Adjust the content access if your message/content model differs.
assert any(
getattr(part, "text", "") and "partial " in getattr(part, "text", "")
for part in getattr(last_msg, "content", []) # e.g. a list of TextPart / content parts
)
```
If your codebase uses a specific content type (e.g. `TextPart`) instead of generic objects with a `.text` attribute, it is better to tighten the assertion:
- Update the `any(...)` check to:
```python
assert any(
isinstance(part, TextPart) and "partial " in part.text
for part in last_msg.content
)
```
- Ensure `TextPart` (or the appropriate class) is imported at the top of `tests/test_tool_loop_agent_runner.py`, e.g.:
```python
from your_package.messages import TextPart
```
Adjust the import path and attribute names (`content`, `text`) to match the actual message model used in the test suite.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The stop logic around
run_agentandToolLoopAgentRunnerhas become a bit convoluted (inline_should_stop_agentchecks plus the_watch_agent_stop_signaltask plusresp.type == "aborted"handling); consider consolidating this into a single, clearly documented stopping path to reduce the chance of races or duplicated cancellation behavior. - In
_save_to_history, theuser_abortedpath has several special cases and a commented‑out message append block; it would be good to either remove the commented code and simplify the role/completion handling, or explicitly define the intended history representation for aborted runs so that this logic is easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The stop logic around `run_agent` and `ToolLoopAgentRunner` has become a bit convoluted (inline `_should_stop_agent` checks plus the `_watch_agent_stop_signal` task plus `resp.type == "aborted"` handling); consider consolidating this into a single, clearly documented stopping path to reduce the chance of races or duplicated cancellation behavior.
- In `_save_to_history`, the `user_aborted` path has several special cases and a commented‑out message append block; it would be good to either remove the commented code and simplify the role/completion handling, or explicitly define the intended history representation for aborted runs so that this logic is easier to reason about.
## Individual Comments
### Comment 1
<location> `tests/test_tool_loop_agent_runner.py:419-428` </location>
<code_context>
+@pytest.mark.asyncio
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen assertions to verify the persisted partial message content, not only the role.
Right now the test only validates `final_resp.completion_text == "partial "` and that the last `run_context` message has role `"assistant"`. To better validate persistence, also assert that the last message’s content (e.g., its `TextPart`) includes the expected text chunk `"partial "`. This ensures we catch regressions where `final_llm_resp` is correct but the `run_context` messages are not updated.
Suggested implementation:
```python
assert final_resp.completion_text == "partial "
messages = runner.run_context.messages
last_msg = messages[-1]
# Ensure the last message is from the assistant
assert last_msg.role == "assistant"
# Ensure the partial text was actually persisted on the last message content
# Adjust the content access if your message/content model differs.
assert any(
getattr(part, "text", "") and "partial " in getattr(part, "text", "")
for part in getattr(last_msg, "content", []) # e.g. a list of TextPart / content parts
)
```
If your codebase uses a specific content type (e.g. `TextPart`) instead of generic objects with a `.text` attribute, it is better to tighten the assertion:
- Update the `any(...)` check to:
```python
assert any(
isinstance(part, TextPart) and "partial " in part.text
for part in last_msg.content
)
```
- Ensure `TextPart` (or the appropriate class) is imported at the top of `tests/test_tool_loop_agent_runner.py`, e.g.:
```python
from your_package.messages import TextPart
```
Adjust the import path and attribute names (`content`, `text`) to match the actual message model used in the test suite.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @pytest.mark.asyncio | ||
| async def test_stop_signal_returns_aborted_and_persists_partial_message( | ||
| runner, provider_request, mock_tool_executor, mock_hooks | ||
| ): | ||
| provider = MockAbortableStreamProvider() | ||
|
|
||
| await runner.reset( | ||
| provider=provider, | ||
| request=provider_request, | ||
| run_context=ContextWrapper(context=None), |
There was a problem hiding this comment.
suggestion (testing): 加强断言，以验证持久化的部分消息内容，而不仅仅是角色。
目前测试只验证了 final_resp.completion_text == "partial "，以及最后一条 run_context 消息的角色是 "assistant"。为了更好地验证持久化结果，建议同时断言最后一条消息的内容（例如其 TextPart）包含预期的文本片段 "partial "。这样可以确保在 final_llm_resp 是正确的但 run_context 消息未更新的情况下，我们也能捕获回归问题。
建议的实现：
assert final_resp.completion_text == "partial "
messages = runner.run_context.messages
last_msg = messages[-1]
# Ensure the last message is from the assistant
assert last_msg.role == "assistant"
# Ensure the partial text was actually persisted on the last message content
# Adjust the content access if your message/content model differs.
assert any(
getattr(part, "text", "") and "partial " in getattr(part, "text", "")
for part in getattr(last_msg, "content", []) # e.g. a list of TextPart / content parts
)如果你的代码库使用的是特定的内容类型（例如 TextPart），而不是带有 .text 属性的通用对象，那么最好进一步收紧断言：
- 将
any(...)检查更新为：
assert any(
isinstance(part, TextPart) and "partial " in part.text
for part in last_msg.content
)- 确保在
tests/test_tool_loop_agent_runner.py文件顶部导入TextPart（或相应的类），例如：
from your_package.messages import TextPart根据测试套件中实际使用的消息模型，调整导入路径以及属性名（content、text）。
Original comment in English
suggestion (testing): Strengthen assertions to verify the persisted partial message content, not only the role.
Right now the test only validates final_resp.completion_text == "partial " and that the last run_context message has role "assistant". To better validate persistence, also assert that the last message’s content (e.g., its TextPart) includes the expected text chunk "partial ". This ensures we catch regressions where final_llm_resp is correct but the run_context messages are not updated.
Suggested implementation:
assert final_resp.completion_text == "partial "
messages = runner.run_context.messages
last_msg = messages[-1]
# Ensure the last message is from the assistant
assert last_msg.role == "assistant"
# Ensure the partial text was actually persisted on the last message content
# Adjust the content access if your message/content model differs.
assert any(
getattr(part, "text", "") and "partial " in getattr(part, "text", "")
for part in getattr(last_msg, "content", []) # e.g. a list of TextPart / content parts
)If your codebase uses a specific content type (e.g. TextPart) instead of generic objects with a .text attribute, it is better to tighten the assertion:
- Update the
any(...)check to:
assert any(
isinstance(part, TextPart) and "partial " in part.text
for part in last_msg.content
)- Ensure
TextPart(or the appropriate class) is imported at the top oftests/test_tool_loop_agent_runner.py, e.g.:
from your_package.messages import TextPartAdjust the import path and attribute names (content, text) to match the actual message model used in the test suite.
1 participant
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
为正在进行的 agent 运行添加用户发起的停止支持，同时确保在聊天会话中保留部分响应和文件附件。
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
Summary by Sourcery
Add user-initiated stop support for ongoing agent runs while ensuring partial responses and file attachments are preserved in chat sessions.
New Features:
Bug Fixes:
Enhancements:
Tests: