-
Notifications
You must be signed in to change notification settings - Fork 219
fix(voice): dont abort tool call when preamble forwarding stops #756
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
Conversation
🦋 Changeset detectedLatest commit: a82ef4e The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Nice finding! Suggest a small improvement (no need to manually create abortController as Task itself will auto spawn a new one if we do not specify inside Task.from:
const messageOutputs: Array<[string, _TextOut | null, _AudioOut | null]> = [];
const tasks = [
Task.from(
(controller) => readMessages(controller, messageOutputs),
undefined,
'AgentActivity.realtime_generation.read_messages',
),
];7bfd499 to
faa9741
Compare
co-authored by Scott Muhs (scott.muhs@assured.claims)
faa9741 to
792f6cb
Compare
toubatbrian
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.
Add some comments
|
One final request: could you double-check the use case you're aiming to implement with this fix? Once you're happy with it, I’ll go ahead and merge. Thanks again for helping dig into and spot this super tricky bug! Also, please add a changeset to patch update the agent-js package. Let me know if you need any help! |
|
absolutely -- i've been testing this morning and all is working well. (now that i updated the controller in the forwarders 🤦 ). i'll add a changeset -- also, would you mind if i open a separate PR to update the contributing.md file? It was confusing because it asked that I commit to the |
|
@toubatbrian added changeset to patch -- thank you for the quick review and help on this! As an aside, the code is very readable and well documented -- it's been a great experience and I very much appreciate the work yall are doing at LiveKit! |
@jjsquillante Yes, please feel free to open a separate PR for that. I got the same ask once and updated the contributing section in the README, but totally forgot to update contributing.md. |
|
Reason as to why this wasn't an issue in python https://live-kit.slack.com/archives/C07TWVC0W4A/p1759946906812589?thread_ts=1759897659.483869&cid=C07TWVC0W4A |
Description
Ensure tool calls are not aborted when preamble text forwarding stops. It refines the execution flow so that cleanup of preamble forwarders does not propagate an abort to in-flight tool executions. It also introduces tests around tool execution behavior.
Changes Made
agents/src/voice/agent_activity.tsagents/src/voice/generation_tools.test.tsPre-Review Checklist
Testing
restaurant_agent.tsandrealtime_agent.tswork properly (for major changes)Additional Notes
Note to reviewers: Please ensure the pre-review checklist is completed before starting your review.