Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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/agent/runners/tool_loop_agent_runner.py" line_range="928" />
<code_context>
await self._complete_with_assistant_response(llm_resp)
+ else:
+ try:
+ await self.agent_hooks.on_agent_response(self.run_context, llm_resp)
+ except Exception as e:
+ logger.error(f"Error in on_agent_response hook: {e}", exc_info=True)
</code_context>
<issue_to_address>
**Streaming chunks bypass response hooks**
When streaming is enabled and a tool-call response contains chunks that an `on_llm_response` handler would modify, `step` yields reasoning and text chunks before `on_agent_response` runs, so handlers cannot modify the already-emitted content and users can see text that differs from the hooked response.
Run response handlers before yielding the tool-call response's streaming chunks.
</issue_to_address>
### Comment 2
<location path="tests/test_tool_loop_agent_runner.py" line_range="935-945" />
<code_context>
+ streaming=False,
+ )
+
+ responses = [response async for response in runner.step()]
+
+ assert len(mock_hooks.agent_responses) == 1
+ assert mock_hooks.agent_responses[0].completion_text == "我需要使用工具来帮助您"
+ assert mock_hooks.agent_responses[0].tools_call_name == ["test_tool"]
+ assert not mock_hooks.agent_done_called
+ assert any(
+ response.type == "llm_result"
+ and response.data["chain"].get_plain_text() == "我需要使用工具来帮助您"
+ for response in responses
+ )
+
+
</code_context>
<issue_to_address>
**Late response hooks still pass**
When the runner emits the intermediate assistant text before dispatching the response hook, `test_tool_call_response_invokes_response_hook` consumes the entire async generator at line 935, then only checks that the hook ran and the text appeared at lines 937–945. If the intermediate text is emitted before `on_agent_response` runs, these assertions still pass, so the test does not catch the ordering regression.
Record the hook and text-emission order and assert the hook occurs first.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: astrbot/core/agent/runners/tool_loop_agent_runner.py:928, tests/test_tool_loop_agent_runner.py:945
|
@sourcery-ai review |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/astr_agent_hooks.py" line_range="19-21" />
<code_context>
+ ) -> bool:
+ event = run_context.context.event
+ return bool(
+ star_handlers_registry.get_handlers_by_event_type(
+ EventType.OnLLMResponseEvent,
+ plugins_name=event.plugins_name,
+ )
+ )
</code_context>
<issue_to_address>
**Incremental output is withheld**
When streaming is enabled and an active response handler only observes the response, `should_buffer_streaming_response` treats any registered `OnLLMResponseEvent` handler, including the built-in persistence handler that only reads the response, as a reason to buffer; users receive no incremental output until generation finishes.
Buffer only when an active response handler can modify the response.
</issue_to_address>
### Comment 2
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="849" />
<code_context>
- data=AgentResponseData(
- chain=MessageChain(type="reasoning").message(
- llm_response.reasoning_content,
+ if not buffer_streaming_response:
+ if llm_response.reasoning_content:
+ yield AgentResponse(
</code_context>
<issue_to_address>
**Partial streamed text disappears**
When buffering is enabled, chunks arrive, and generation stops, errors, or fails to produce a complete response, `step` discards the chunks while buffering and exits through the abort or error path without flushing them, so clients receive none of the partial text.
Retain buffered chunks and emit them when the stream ends without a complete response.
</issue_to_address>
### Comment 3
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="933" />
<code_context>
await self._complete_with_assistant_response(llm_resp)
+ else:
+ try:
+ await self.agent_hooks.on_agent_response(self.run_context, llm_resp)
+ except Exception as e:
+ logger.error(f"Error in on_agent_response hook: {e}", exc_info=True)
</code_context>
<issue_to_address>
**Cleared tool calls leave agent running**
When an `OnLLMResponseEvent` handler removes all tool calls from the mutable response, a response hook can clear `llm_resp.tools_call_name` after the earlier branch selected the tool-call path. The later tool check then skips execution, but `_complete_with_assistant_response` was also skipped, leaving the runner in `RUNNING` and causing another agent step instead of finalizing the response.
After the response hook runs, re-evaluate the response and finalize it as an assistant response if no tool calls remain.
</issue_to_address>
### Comment 4
<location path="astrbot/core/astr_agent_hooks.py" line_range="92-95" />
<code_context>
class BaseAgentRunHooks(Generic[TContext]):
+ def should_buffer_streaming_response(
+ self, run_context: ContextWrapper[TContext]
+ ) -> bool:
+ """Whether response hooks may modify streamed response content."""
+ return False
+
async def on_agent_begin(self, run_context: ContextWrapper[TContext]) -> None: ...
</code_context>
<issue_to_address>
**Empty hooks duplicate inherited behavior**
When `EmptyAgentHooks` inherits from `BaseAgentRunHooks`, `EmptyAgentHooks.should_buffer_streaming_response` only repeats the base implementation's `False` return, so it adds an override without changing behavior or solving a separate need.
Remove the redundant override and inherit the base implementation.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and a response hook can now modify an intermediate tool-call response before the runner uses its tool name and arguments, potentially changing actions that a tool performs. Reverting stops future dispatches, but cannot undo external side effects from tool calls already made.
Blocking findings: astrbot/core/astr_agent_hooks.py:21, astrbot/core/agent/runners/tool_loop_agent_runner.py:849, astrbot/core/agent/runners/tool_loop_agent_runner.py:933
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="949" />
<code_context>
await self._complete_with_assistant_response(llm_resp)
+ else:
+ try:
+ await self.agent_hooks.on_agent_response(self.run_context, llm_resp)
+ except Exception as e:
+ logger.error(f"Error in on_agent_response hook: {e}", exc_info=True)
</code_context>
<issue_to_address>
**Canceled requests still execute tools**
When the runner task is canceled while an `OnLLMResponseEvent` handler is running, `call_event_hook` catches `BaseException`, including `asyncio.CancelledError`, from a plugin handler and suppresses it. The runner then continues past the response hook into tool execution, so a canceled request can still perform tool side effects.
Re-raise `asyncio.CancelledError` from `call_event_hook` so task cancellation propagates before tool execution.
</issue_to_address>
### Comment 2
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="860" />
<code_context>
),
)
+ if buffer_streaming_response:
+ buffered_streaming_responses.append(response)
+ else:
+ yield response
</code_context>
<issue_to_address>
**Long streams retain every chunk**
When a hooked streaming response contains a very large number of chunks, with an external response handler enabled, the runner retains a separate `AgentResponse` for every streamed chunk until the provider finishes. Very long outputs therefore accumulate unbounded per-chunk objects in memory, increasing memory pressure or exhausting the agent process.
Bound or compact buffered partial output while preserving the ability to flush it on failure or stop.
</issue_to_address>
### Comment 3
<location path="tests/test_tool_loop_agent_runner.py" line_range="1133" />
<code_context>
+ run_context = ContextWrapper(context=SimpleNamespace(event=event))
+ monkeypatch.setattr(
+ "astrbot.core.astr_agent_hooks.star_handlers_registry.get_handlers_by_event_type",
+ lambda *args, **kwargs: [handler],
+ )
+ monkeypatch.setitem(star_map, module_path, SimpleNamespace(reserved=reserved))
</code_context>
<issue_to_address>
**Response handlers go undetected**
When the registry lookup uses the wrong event type or omits the plugin filter, `test_main_agent_buffers_only_for_external_response_handlers` replaces the registry lookup with a lambda that ignores its arguments, so the test passes if `should_buffer_streaming_response` queries the wrong event type or omits the plugin filter. The production code can then miss response handlers or buffer for unrelated handlers without this test failing.
Assert the registry lookup receives `EventType.OnLLMResponseEvent` and the event's `plugins_name`, or test against a registry containing handlers for distinct event types.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and a registered response handler can now change a tool-call response before the runner executes it, potentially selecting a different tool or arguments; any external action that tool performs is not undone by reverting this change.
Blocking findings: astrbot/core/agent/runners/tool_loop_agent_runner.py:949, astrbot/core/agent/runners/tool_loop_agent_runner.py:860, tests/test_tool_loop_agent_runner.py:1133
| await self._complete_with_assistant_response(llm_resp) | ||
| else: | ||
| try: | ||
| await self.agent_hooks.on_agent_response(self.run_context, llm_resp) |
There was a problem hiding this comment.
🟠 High · Canceled requests still execute tools
When the runner task is canceled while an OnLLMResponseEvent handler is running, call_event_hook catches BaseException, including asyncio.CancelledError, from a plugin handler and suppresses it. The runner then continues past the response hook into tool execution, so a canceled request can still perform tool side effects.
Re-raise asyncio.CancelledError from call_event_hook so task cancellation propagates before tool execution.
Prompt for AI agents
In `astrbot/core/agent/runners/tool_loop_agent_runner.py` at line 949:
**Canceled requests still execute tools**
When the runner task is canceled while an `OnLLMResponseEvent` handler is running, `call_event_hook` catches `BaseException`, including `asyncio.CancelledError`, from a plugin handler and suppresses it. The runner then continues past the response hook into tool execution, so a canceled request can still perform tool side effects.
Re-raise `asyncio.CancelledError` from `call_event_hook` so task cancellation propagates before tool execution.| ), | ||
| ) | ||
| if buffer_streaming_response: | ||
| buffered_streaming_responses.append(response) |
There was a problem hiding this comment.
🟡 Medium · Long streams retain every chunk
When a hooked streaming response contains a very large number of chunks, with an external response handler enabled, the runner retains a separate AgentResponse for every streamed chunk until the provider finishes. Very long outputs therefore accumulate unbounded per-chunk objects in memory, increasing memory pressure or exhausting the agent process.
Bound or compact buffered partial output while preserving the ability to flush it on failure or stop.
Prompt for AI agents
In `astrbot/core/agent/runners/tool_loop_agent_runner.py` at line 860:
**Long streams retain every chunk**
When a hooked streaming response contains a very large number of chunks, with an external response handler enabled, the runner retains a separate `AgentResponse` for every streamed chunk until the provider finishes. Very long outputs therefore accumulate unbounded per-chunk objects in memory, increasing memory pressure or exhausting the agent process.
Bound or compact buffered partial output while preserving the ability to flush it on failure or stop.| run_context = ContextWrapper(context=SimpleNamespace(event=event)) | ||
| monkeypatch.setattr( | ||
| "astrbot.core.astr_agent_hooks.star_handlers_registry.get_handlers_by_event_type", | ||
| lambda *args, **kwargs: [handler], |
There was a problem hiding this comment.
🟡 Medium · Response handlers go undetected
When the registry lookup uses the wrong event type or omits the plugin filter, test_main_agent_buffers_only_for_external_response_handlers replaces the registry lookup with a lambda that ignores its arguments, so the test passes if should_buffer_streaming_response queries the wrong event type or omits the plugin filter. The production code can then miss response handlers or buffer for unrelated handlers without this test failing.
Assert the registry lookup receives EventType.OnLLMResponseEvent and the event's plugins_name, or test against a registry containing handlers for distinct event types.
Prompt for AI agents
In `tests/test_tool_loop_agent_runner.py` at line 1133:
**Response handlers go undetected**
When the registry lookup uses the wrong event type or omits the plugin filter, `test_main_agent_buffers_only_for_external_response_handlers` replaces the registry lookup with a lambda that ignores its arguments, so the test passes if `should_buffer_streaming_response` queries the wrong event type or omits the plugin filter. The production code can then miss response handlers or buffer for unrelated handlers without this test failing.
Assert the registry lookup receives `EventType.OnLLMResponseEvent` and the event's `plugins_name`, or test against a registry containing handlers for distinct event types.
Dispatch
OnLLMResponseEventbefore streaming tool-call text when external handlers can rewrite it. Preserve buffered partial output on cancellation or failure, finalize responses when hooks remove all tool calls, and keep reserved core observers from delaying streaming.Validation:
uv run pytest tests/test_tool_loop_agent_runner.py -q— 67 passeduv run ruff check astrbot/core/astr_agent_hooks.py astrbot/core/agent/runners/tool_loop_agent_runner.py tests/test_tool_loop_agent_runner.py— passedgit diff --check— passedFixes #10770. AI assistance was used and the changes were reviewed.
Summary by Sourcery
Ensure tool-call response hooks can safely rewrite streamed output without losing partial responses or delaying unaffected streams.
Bug Fixes:
Enhancements:
Tests: