feat: pass client tool results back to engine - #54
Conversation
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/anam/_tool_call_manager.py">
<violation number="1" location="src/anam/_tool_call_manager.py:153">
P1: Client tool handlers that return `None` are now incorrectly treated as completed results, which breaks the documented `on_start` contract and can prematurely complete fire-and-forget tool calls.</violation>
</file>
<file name="src/anam/client.py">
<violation number="1" location="src/anam/client.py:274">
P2: `connect_async`'s new failure cleanup is partial; it should also tear down/reset streaming/session state when `connect()` raises.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| self._send_client_tool_result(event, result=result, error_message=None) | ||
| # Auto-complete with the returned result | ||
| completed_payload = self._build_completed_payload(event, result=result) | ||
| await self._emit(AnamEvent.TOOL_CALL_COMPLETED, completed_payload) | ||
| if tool_call_id: | ||
| self._pending_calls.pop(tool_call_id, None) | ||
| # Dispatch to handler's on_complete callback | ||
| if hasattr(handler, "on_complete") and handler.on_complete is not None: | ||
| try: | ||
| await handler.on_complete(completed_payload) | ||
| except Exception as cb_err: | ||
| logger.error( | ||
| "Error in on_complete handler for tool '%s': %s", | ||
| tool_name, | ||
| cb_err, | ||
| ) |
There was a problem hiding this comment.
P1: Client tool handlers that return None are now incorrectly treated as completed results, which breaks the documented on_start contract and can prematurely complete fire-and-forget tool calls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/anam/_tool_call_manager.py, line 153:
<comment>Client tool handlers that return `None` are now incorrectly treated as completed results, which breaks the documented `on_start` contract and can prematurely complete fire-and-forget tool calls.</comment>
<file context>
@@ -108,31 +147,36 @@ async def process_started_event(self, event: dict[str, Any]) -> None:
- tool_name,
- cb_err,
- )
+ self._send_client_tool_result(event, result=result, error_message=None)
+ # Auto-complete with the returned result
+ completed_payload = self._build_completed_payload(event, result=result)
</file context>
| self._send_client_tool_result(event, result=result, error_message=None) | |
| # Auto-complete with the returned result | |
| completed_payload = self._build_completed_payload(event, result=result) | |
| await self._emit(AnamEvent.TOOL_CALL_COMPLETED, completed_payload) | |
| if tool_call_id: | |
| self._pending_calls.pop(tool_call_id, None) | |
| # Dispatch to handler's on_complete callback | |
| if hasattr(handler, "on_complete") and handler.on_complete is not None: | |
| try: | |
| await handler.on_complete(completed_payload) | |
| except Exception as cb_err: | |
| logger.error( | |
| "Error in on_complete handler for tool '%s': %s", | |
| tool_name, | |
| cb_err, | |
| ) | |
| if result is not None: | |
| self._send_client_tool_result(event, result=result, error_message=None) | |
| # Auto-complete with the returned result | |
| completed_payload = self._build_completed_payload(event, result=result) | |
| await self._emit(AnamEvent.TOOL_CALL_COMPLETED, completed_payload) | |
| if tool_call_id: | |
| self._pending_calls.pop(tool_call_id, None) | |
| # Dispatch to handler's on_complete callback | |
| if hasattr(handler, "on_complete") and handler.on_complete is not None: | |
| try: | |
| await handler.on_complete(completed_payload) | |
| except Exception as cb_err: | |
| logger.error( | |
| "Error in on_complete handler for tool '%s': %s", | |
| tool_name, | |
| cb_err, | |
| ) |
There was a problem hiding this comment.
This is ok, for fire and forget tools this result passback get dropped on the engine side, if await_result is true we always need to send this event (even if there is no returned value)
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
| try: | ||
| await self._streaming_client.connect() | ||
| except Exception: | ||
| self._tool_call_manager.clear_session_state() |
There was a problem hiding this comment.
P2: connect_async's new failure cleanup is partial; it should also tear down/reset streaming/session state when connect() raises.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/anam/client.py, line 274:
<comment>`connect_async`'s new failure cleanup is partial; it should also tear down/reset streaming/session state when `connect()` raises.</comment>
<file context>
@@ -262,8 +262,17 @@ async def connect_async(self, session_options: SessionOptions = SessionOptions()
+ try:
+ await self._streaming_client.connect()
+ except Exception:
+ self._tool_call_manager.clear_session_state()
+ raise
self._is_streaming = True
</file context>
Summary by cubic
Send client tool results back to the engine so the LLM can use them in its reply. Adds session-scoped event filtering and tool config options for awaiting results and timeouts.
New Features
Migration
Written for commit b1da5d0. Summary will update on new commits.