Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the robustness of the agent's reasoning loop by ensuring terminal assistant messages are persisted during cancellations, LLM failures, and when reaching maximum iterations. It introduces a helper function for message persistence, improves error handling for subagents, and updates the terminal UI to display character counts for tool results. Additionally, the memory module's message retrieval order was corrected to follow insertion order, and several mutex poisoning handling improvements were implemented across the codebase. The review comment correctly identifies a significant opportunity to reduce code duplication in the cancellation handling logic using a macro.
| if cancel_token.is_cancelled() { | ||
| persist_terminal_assistant_message( | ||
| &mut mem, | ||
| &logger_tx, | ||
| &name, | ||
| &inbound.chat_id, | ||
| cancel_notice, | ||
| ) | ||
| .await; | ||
| return Ok(String::new()); | ||
| } | ||
| let tool_result = match fin { | ||
| ToolExecutionFinished::Completed(res) => res, | ||
| ToolExecutionFinished::Cancelled => return Ok(String::new()), | ||
| ToolExecutionFinished::Cancelled => { | ||
| persist_terminal_assistant_message( | ||
| &mut mem, | ||
| &logger_tx, | ||
| &name, | ||
| &inbound.chat_id, | ||
| cancel_notice, | ||
| ) | ||
| .await; | ||
| return Ok(String::new()); | ||
| } |
There was a problem hiding this comment.
There's a lot of repetition in the cancellation handling logic. The block for persist_terminal_assistant_message followed by return Ok(String::new()) is duplicated in many places within this function (e.g., lines 1412-1419, 1549-1557, 1653-1661, 1736-1743, 1749-1757, 1785-1793, 1851-1860, 1990-1998).
To improve maintainability and reduce code duplication, you could define a local macro to encapsulate this logic. This would make the code cleaner and less error-prone if this logic needs to be changed in the future.
Example of a macro:
macro_rules! persist_and_cancel {
() => {{
persist_terminal_assistant_message(
&mut mem,
&logger_tx,
&name,
&inbound.chat_id,
cancel_notice,
)
.await;
return Ok(String::new());
}};
}Then you could use it like persist_and_cancel!() in all the cancellation handling branches.
No description provided.