Fix eager turn detection updating transcripts out of order#401
Merged
Fix eager turn detection updating transcripts out of order#401
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces explicit turn lifecycle handling: adds Changes
Sequence DiagramsequenceDiagram
participant User as User Input
participant Agent as Agent
participant LLM as LLM
participant Conv as Conversation
participant TTS as TTS
User->>Agent: TurnStartedEvent
Agent->>Agent: _on_turn_started()
User->>Agent: Partial transcript / LLM deltas
alt pending turn exists
Agent-->>LLM: early-return / hold updates
else
Agent->>LLM: process deltas
Agent->>Conv: update conversation with partial transcript
end
User->>Agent: TurnEndedEvent
Agent->>Agent: _on_turn_ended()
Agent->>Agent: create/update LLMTurn (or finalize eager turn)
Agent->>Agent: _finish_llm_turn()
Agent->>Conv: write assistant message
Agent->>TTS: _flush_streaming_tts_buffer()
TTS->>User: Playback assistant response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Guard too narrow causes duplicate assistant message writes
- I made
_finish_llm_turnwrite to the conversation only for deferred eager-turn completion, so normal completed-response events are written once by the sync handler and no longer double-written.
- I made
Or push these changes by commenting:
@cursor push 9277429080
Preview (9277429080)
diff --git a/agents-core/vision_agents/core/agents/agents.py b/agents-core/vision_agents/core/agents/agents.py
--- a/agents-core/vision_agents/core/agents/agents.py
+++ b/agents-core/vision_agents/core/agents/agents.py
@@ -331,7 +331,7 @@
else:
self._pending_turn.response = event
if self._pending_turn.turn_finished:
- await self._finish_llm_turn()
+ await self._finish_llm_turn(write_to_conversation=False)
else:
# we are in eager turn completion mode. wait for confirmation
self._pending_turn.response = event
@@ -1647,14 +1647,14 @@
# Same text as pending turn — confirm the eager turn
self._pending_turn.turn_finished = True
if self._pending_turn.response is not None:
- await self._finish_llm_turn()
+ await self._finish_llm_turn(write_to_conversation=True)
@property
def _has_pending_eager_turn(self) -> bool:
"""True when an eager turn is in flight and not yet confirmed."""
return self._pending_turn is not None and not self._pending_turn.turn_finished
- async def _finish_llm_turn(self):
+ async def _finish_llm_turn(self, write_to_conversation: bool):
if self._pending_turn is None or self._pending_turn.response is None:
raise ValueError(
"Finish LLM turn should only be called after self._pending_turn is set"
@@ -1667,7 +1667,7 @@
# The user message was already written by on_stt_transcript_event_sync_conversation
# which fires on the final STTTranscriptEvent before TurnEndedEvent.
assert self.agent_user.id
- if self.conversation is not None and event and event.text:
+ if write_to_conversation and self.conversation is not None and event and event.text:
await self.conversation.upsert_message(
message_id=event.item_id,
role="assistant",![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
agents-core/vision_agents/core/agents/agents.py (2)
1625-1628: Useself.loggerinstead of module-levellogger.The rest of the file uses
self.logger.debug(...)for instance-level logging with the agent ID context. This line uses the bare module logger, losing that context. As per coding guidelines, when adding code to an existing file, follow the patterns already established in that file.♻️ Proposed fix
- logger.debug( + self.logger.debug( "Eager turn and completed turn didn't match. Cancelling in flight response. %s vs %s ", self._pending_turn.input, transcript, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents-core/vision_agents/core/agents/agents.py` around lines 1625 - 1628, Replace the module-level logger call with the instance logger so the agent ID/context is preserved: change the call that currently uses logger.debug(...) (the one logging "Eager turn and completed turn didn't match. Cancelling in flight response. %s vs %s " with self._pending_turn.input and transcript) to use self.logger.debug(...) and keep the same message and parameters; this maintains consistent per-agent logging behavior in the method that references self._pending_turn and transcript.
1669-1669: Replaceassertwith explicit validation.
assertstatements are stripped when Python runs with-O(optimize), which could silently allowNoneto pass through in production. RaiseValueErrorinstead for runtime preconditions.♻️ Proposed fix
- assert self.agent_user.id + if not self.agent_user.id: + raise ValueError("agent_user.id must be set before finishing LLM turn") if self.conversation is not None and event and event.text:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents-core/vision_agents/core/agents/agents.py` at line 1669, Replace the runtime-only assert with explicit validation: in the method containing the line `assert self.agent_user.id` check `if not self.agent_user or not self.agent_user.id:` and raise a ValueError with a clear message (e.g., "agent_user.id must be set") instead of using assert so the precondition is enforced in production; update any surrounding logic that assumes `agent_user.id` to rely on this validation.tests/test_agents/test_turndetection.py (2)
120-127: Add cleanup to prevent resource leakage.The test ends without calling
agent.close(), leaving event handlers and internal state dangling. While pytest may isolate this, explicit cleanup avoids subtle accumulation issues across test runs.♻️ Proposed cleanup
assert roles.index("user") < roles.index("assistant"), ( f"User message should come before assistant. Got: {roles}" ) + + await agent.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_agents/test_turndetection.py` around lines 120 - 127, The test leaves the Agent instance running and should explicitly close it to avoid leaking event handlers/state: after the assertions in the test (where you inspect agent.conversation.messages and roles), call agent.close() (or wrap the test body in try/finally and call agent.close() in finally) so the Agent's resources are cleaned up; ensure the call targets the agent.close() method on the same Agent instance used in the test.
102-104: Consider replacing sleep with deterministic synchronization.
asyncio.sleep(0.1)introduces timing-based flakiness—the test could fail on slower CI runners or pass despite bugs if timing shifts. Ifagent._pending_turn.taskis accessible, awaiting it directly would be more reliable.♻️ Suggested alternative
await agent.events.wait() - # Let the LLM task (created via asyncio.create_task) complete - await asyncio.sleep(0.1) + # Wait for the LLM task to complete deterministically + if agent._pending_turn and agent._pending_turn.task: + await agent._pending_turn.task await agent.events.wait()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_agents/test_turndetection.py` around lines 102 - 104, The test currently uses asyncio.sleep(0.1) which is flaky; instead wait deterministically for the LLM task created on the agent by awaiting the actual task object (agent._pending_turn.task) or using asyncio.wait_for(agent._pending_turn.task, timeout=...) to avoid hangs, then keep the existing await agent.events.wait() if needed for event propagation; update the test to access agent._pending_turn.task (or check for its existence first) and await it rather than sleeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@agents-core/vision_agents/core/agents/agents.py`:
- Around line 1625-1628: Replace the module-level logger call with the instance
logger so the agent ID/context is preserved: change the call that currently uses
logger.debug(...) (the one logging "Eager turn and completed turn didn't match.
Cancelling in flight response. %s vs %s " with self._pending_turn.input and
transcript) to use self.logger.debug(...) and keep the same message and
parameters; this maintains consistent per-agent logging behavior in the method
that references self._pending_turn and transcript.
- Line 1669: Replace the runtime-only assert with explicit validation: in the
method containing the line `assert self.agent_user.id` check `if not
self.agent_user or not self.agent_user.id:` and raise a ValueError with a clear
message (e.g., "agent_user.id must be set") instead of using assert so the
precondition is enforced in production; update any surrounding logic that
assumes `agent_user.id` to rely on this validation.
In `@tests/test_agents/test_turndetection.py`:
- Around line 120-127: The test leaves the Agent instance running and should
explicitly close it to avoid leaking event handlers/state: after the assertions
in the test (where you inspect agent.conversation.messages and roles), call
agent.close() (or wrap the test body in try/finally and call agent.close() in
finally) so the Agent's resources are cleaned up; ensure the call targets the
agent.close() method on the same Agent instance used in the test.
- Around line 102-104: The test currently uses asyncio.sleep(0.1) which is
flaky; instead wait deterministically for the LLM task created on the agent by
awaiting the actual task object (agent._pending_turn.task) or using
asyncio.wait_for(agent._pending_turn.task, timeout=...) to avoid hangs, then
keep the existing await agent.events.wait() if needed for event propagation;
update the test to access agent._pending_turn.task (or check for its existence
first) and await it rather than sleeping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e81a2081-4b62-4e64-91a0-f86604d1136d
📒 Files selected for processing (2)
agents-core/vision_agents/core/agents/agents.pytests/test_agents/test_turndetection.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
agents-core/vision_agents/core/agents/agents.py (3)
1575-1579: Redundant conditional —event.participantis guaranteed truthy here.Within this branch, the outer
if event.participant and ...already ensuresevent.participantis notNone. The ternary guard is superfluous.♻️ Simplify by using participant directly
else: - user_id = event.participant.user_id if event.participant else "unknown" + user_id = event.participant.user_id self.logger.info( '👉 Turn started - participant "%s" is speaking', user_id )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents-core/vision_agents/core/agents/agents.py` around lines 1575 - 1579, The ternary fallback in the else branch is redundant because the outer conditional already guarantees event.participant is truthy; replace the conditional assignment user_id = event.participant.user_id if event.participant else "unknown" with a direct use of event.participant.user_id (or the existing participant variable from the outer check) and update the logger call in the same block to reference that direct user_id, removing the unnecessary None-guard.
1603-1606: Magic sleep value — consider documenting or making configurable.The 20ms sleep to allow STT to catch up is a timing heuristic that may be insufficient under load or with slower STT providers. Consider adding a comment explaining the rationale or extracting to a named constant.
📝 Document the delay
if not event.eager_end_of_turn and self.stt: await self.stt.clear() - # give the speech to text a moment to catch up + # Allow STT pipeline to flush final transcripts before reading buffer. + # 20ms is empirically sufficient for most providers; may need tuning. await asyncio.sleep(0.02)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents-core/vision_agents/core/agents/agents.py` around lines 1603 - 1606, The 20ms hardcoded sleep after awaiting self.stt.clear() is a magic timing heuristic; replace it with a named constant (e.g., STT_CATCHUP_DELAY_MS or STT_CATCHUP_DELAY) or read from configuration and add a short comment explaining why the delay exists and that it may need adjustment for slower STT providers; update the await asyncio.sleep(0.02) call in the block that checks event.eager_end_of_turn and self.stt to use the constant/setting (converted to seconds) and document the rationale near the event.eager_end_of_turn / self.stt.clear() logic.
1652-1664: Replaceassertwith explicit validation; add docstring.The
assertat line 1664 can be stripped when Python runs with-O. Since__init__guaranteesagent_user.idis set, the assertion is defensive but still not production-safe. Additionally, this method lacks a Google-style docstring per coding guidelines.🛡️ Proposed fix
async def _finish_llm_turn(self): + """Finalize a pending LLM turn and sync the assistant response to conversation. + + Raises: + ValueError: If called without a pending turn or response. + """ if self._pending_turn is None or self._pending_turn.response is None: raise ValueError( "Finish LLM turn should only be called after self._pending_turn is set" ) turn = self._pending_turn self._pending_turn = None event = turn.response # Write deferred assistant response to conversation. # The user message was already written by on_stt_transcript_event_sync_conversation # which fires on the final STTTranscriptEvent before TurnEndedEvent. - assert self.agent_user.id + if not self.agent_user.id: + raise ValueError("agent_user.id must be set") if self.conversation is not None and event and event.text:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents-core/vision_agents/core/agents/agents.py` around lines 1652 - 1664, In _finish_llm_turn, replace the runtime-removed assertion with an explicit validation of self.agent_user.id (e.g., if not self.agent_user.id: raise ValueError("agent_user.id is required in _finish_llm_turn")) and add a Google-style docstring to the _finish_llm_turn method describing its purpose, inputs (uses self._pending_turn and self._pending_turn.response), behavior (writes deferred assistant response to conversation) and possible exceptions; keep the rest of the logic unchanged and reference the method name _finish_llm_turn, the attributes self._pending_turn and self.agent_user.id when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents-core/vision_agents/core/agents/agents.py`:
- Around line 1625-1629: The log call uses the module-level logger instead of
the agent-adapter; replace the use of logger with self.logger in the debug call
that compares self._pending_turn.input and transcript so the message includes
the agent context/prefix from _AgentLoggerAdapter; update the call site where
logger.debug(...) is invoked (the one that logs "Eager turn and completed turn
didn't match...") to use self.logger.debug(...) and keep the existing message
and interpolation arguments unchanged.
---
Nitpick comments:
In `@agents-core/vision_agents/core/agents/agents.py`:
- Around line 1575-1579: The ternary fallback in the else branch is redundant
because the outer conditional already guarantees event.participant is truthy;
replace the conditional assignment user_id = event.participant.user_id if
event.participant else "unknown" with a direct use of event.participant.user_id
(or the existing participant variable from the outer check) and update the
logger call in the same block to reference that direct user_id, removing the
unnecessary None-guard.
- Around line 1603-1606: The 20ms hardcoded sleep after awaiting
self.stt.clear() is a magic timing heuristic; replace it with a named constant
(e.g., STT_CATCHUP_DELAY_MS or STT_CATCHUP_DELAY) or read from configuration and
add a short comment explaining why the delay exists and that it may need
adjustment for slower STT providers; update the await asyncio.sleep(0.02) call
in the block that checks event.eager_end_of_turn and self.stt to use the
constant/setting (converted to seconds) and document the rationale near the
event.eager_end_of_turn / self.stt.clear() logic.
- Around line 1652-1664: In _finish_llm_turn, replace the runtime-removed
assertion with an explicit validation of self.agent_user.id (e.g., if not
self.agent_user.id: raise ValueError("agent_user.id is required in
_finish_llm_turn")) and add a Google-style docstring to the _finish_llm_turn
method describing its purpose, inputs (uses self._pending_turn and
self._pending_turn.response), behavior (writes deferred assistant response to
conversation) and possible exceptions; keep the rest of the logic unchanged and
reference the method name _finish_llm_turn, the attributes self._pending_turn
and self.agent_user.id when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83fcea16-18f7-4672-a444-85f5eedaa71d
📒 Files selected for processing (1)
agents-core/vision_agents/core/agents/agents.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
agents-core/vision_agents/core/agents/agents.py (2)
1644-1671: Consider removing the assert on line 1656.The
assert self.agent_user.idis redundant—__init__guarantees a non-empty ID (lines 173-174 set a UUID fallback if empty). Based on learnings, prefer trusting static type annotations over runtime assertions for internal code.♻️ Remove redundant assertion
turn = self._pending_turn self._pending_turn = None event = turn.response # Write deferred assistant response to conversation. # The user message was already written by on_stt_transcript_event_sync_conversation # which fires on the final STTTranscriptEvent before TurnEndedEvent. - assert self.agent_user.id if self.conversation is not None and event and event.text: await self.conversation.upsert_message( message_id=event.item_id, role="assistant", - user_id=self.agent_user.id, + user_id=self.agent_user.id, # Guaranteed non-empty by __init__ content=event.text, completed=True, replace=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents-core/vision_agents/core/agents/agents.py` around lines 1644 - 1671, Remove the redundant runtime assertion in _finish_llm_turn: delete the line "assert self.agent_user.id" inside the async def _finish_llm_turn method in agents.py and rely on the constructor logic that ensures agent_user.id is populated (the UUID fallback set in __init__). No other behavioral change is needed—leave the conversation.upsert_message and subsequent TTS logic intact.
1559-1582: Redundant ternaries within guarded block.Lines 1573 and 1581 use
event.participant.user_id if event.participant else "unknown", but both are inside a block whereevent.participantis already confirmed truthy (line 1565 guard). The ternaries will never take the"unknown"branch here.♻️ Simplify by removing dead branches
if self.tts: self.logger.info( f"👉 Turn started - interrupting agent for participant {event.participant.user_id}" ) await self.tts.stop_audio() self._streaming_tts_buffer = "" else: - user_id = event.participant.user_id if event.participant else "unknown" self.logger.info( - '👉 Turn started - participant "%s" is speaking', user_id + '👉 Turn started - participant "%s" is speaking', event.participant.user_id ) if self._audio_track is not None: await self._audio_track.flush() else: # Agent itself started speaking - this is normal - user_id = event.participant.user_id if event.participant else "unknown" - self.logger.debug(f"👉 Turn started - agent speaking {user_id}") + user_id = event.participant.user_id if event.participant else "unknown" + self.logger.debug(f"👉 Turn started - agent speaking {user_id}")Note: The else branch (lines 1579-1582) can legitimately have
event.participant = None, so the ternary there is valid. Only lines 1573-1575 are inside the truthy guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents-core/vision_agents/core/agents/agents.py` around lines 1559 - 1582, In _on_turn_started, inside the branch guarded by "if event.participant and event.participant.user_id != self.agent_user.id", remove the redundant ternary expressions that check event.participant again (e.g., replace "event.participant.user_id if event.participant else 'unknown'" with direct access to event.participant.user_id) when setting user_id for logging and when interrupting TTS/_streaming_tts_buffer; keep the ternary only in the outer else branch where event.participant may be None, and ensure references to self.tts, await self.tts.stop_audio(), await self._audio_track.flush(), and self._streaming_tts_buffer remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@agents-core/vision_agents/core/agents/agents.py`:
- Around line 1644-1671: Remove the redundant runtime assertion in
_finish_llm_turn: delete the line "assert self.agent_user.id" inside the async
def _finish_llm_turn method in agents.py and rely on the constructor logic that
ensures agent_user.id is populated (the UUID fallback set in __init__). No other
behavioral change is needed—leave the conversation.upsert_message and subsequent
TTS logic intact.
- Around line 1559-1582: In _on_turn_started, inside the branch guarded by "if
event.participant and event.participant.user_id != self.agent_user.id", remove
the redundant ternary expressions that check event.participant again (e.g.,
replace "event.participant.user_id if event.participant else 'unknown'" with
direct access to event.participant.user_id) when setting user_id for logging and
when interrupting TTS/_streaming_tts_buffer; keep the ternary only in the outer
else branch where event.participant may be None, and ensure references to
self.tts, await self.tts.stop_audio(), await self._audio_track.flush(), and
self._streaming_tts_buffer remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9cd068cb-324d-4819-8e61-f337299b1ef3
📒 Files selected for processing (1)
agents-core/vision_agents/core/agents/agents.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents-core/vision_agents/core/agents/agents.py`:
- Line 1655: Replace the bare assert on self.agent_user.id with explicit
validation that raises a clear exception; instead of "assert self.agent_user.id"
check "if not self.agent_user or not getattr(self.agent_user, 'id', None):" and
raise a ValueError (or RuntimeError) with a descriptive message like
"agent_user.id is required for conversation write" so the code path that
performs the conversation write (referenced around the conversation write call
that follows the assert) fails deterministically in production.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7398ae7a-d7c5-4643-b6b3-d2a259ea2e59
📒 Files selected for processing (1)
agents-core/vision_agents/core/agents/agents.py
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Nash0x7E2
reviewed
Mar 9, 2026
Nash0x7E2
approved these changes
Mar 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The agent was syncing agent transcripts even when the conversation turn was not yet confirmed, which led to race conditions when the agent's transcripts appeared in the chat before the user ones were confirmed
Note
Medium Risk
Touches turn-detection/LLM/TTS coordination and async task cancellation, which can impact conversation ordering and barge-in behavior under load. Added regression test reduces risk but concurrency edge-cases remain possible.
Overview
Fixes eager end-of-turn race conditions by gating assistant conversation writes behind a pending-turn lifecycle, ensuring the user transcript is committed before the assistant message.
Turn handling is refactored into explicit
TurnStartedEvent/TurnEndedEventhooks, with safer in-flight LLM task cancellation (cancel_and_wait) and a_finish_llm_turnpath that performs the final conversation upsert + TTS flush while keeping_pending_turnset to prevent duplicate delta/completed writes.Adds a regression test that simulates partial/final STT transcripts plus eager/final turn-end events and asserts user messages appear before assistant messages.
Written by Cursor Bugbot for commit 993c674. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Refactor
Tests