-
Notifications
You must be signed in to change notification settings - Fork 2.7k
prevent tool cancellation when AgentTask is called inside it #4586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis PR adds a SpeechHandle Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent
participant Speech as SpeechHandle
participant Tool as Tool Task
participant Finally as Finally
Agent->>Speech: read tool_cancelable (old_state)
Agent->>Speech: set tool_cancelable = False
Note over Speech: prevents mid-execution cancellation
Agent->>Tool: create named asyncio task (tool execution)
Tool->>Tool: run tool logic
Note over Tool: task runs without being cancelled by speech pause
Tool-->>Agent: tool completes / returns result
Finally->>Speech: restore tool_cancelable = old_state
Note over Speech: original cancelability restored
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
chenghao-mou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I tested with the email example, and it worked.
| def resume(self) -> None: | ||
| if self.closed: | ||
| logger.warning("_SegmentSynchronizerImpl.resume called after close") | ||
| # logger.warning("_SegmentSynchronizerImpl.resume called after close") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been bugging me for a while!
| # there should be only one message | ||
| msg_gen, text_out, audio_out = message_outputs[0] | ||
| if msg_gen: | ||
| forwarded_text = text_out.text if text_out else "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably remove this line
| self._allow_interruptions = value | ||
|
|
||
| @property | ||
| def tool_cancelable(self) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment here? It wasn't immediately clear to me why a speech handle has anything to do with the tool execution.
when there is a speech generated alongside a tool call, the interruption to the speech shouldn't cancel the tool execution if it's await for an AgentTask.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.