Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e818c727c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await asyncio.wait( | ||
| {self._done_fut, self._interrupt_fut}, | ||
| return_when=asyncio.FIRST_COMPLETED, |
There was a problem hiding this comment.
When an interruption wins this wait, await handle now returns while _done_fut is still pending, as the new test explicitly asserts. However, AgentSession.generate_reply() documents that awaiting the handle waits for the reply to finish and directs callers to inspect handle.exception() afterward; that method raises InvalidStateError until _done_fut completes. Consequently, callers following the public contract can fail whenever speech is interrupted, so either provide a separate completion wait for exception inspection or update the API contract and make post-await inspection safe.
Useful? React with 👍 / 👎.
…tion aware of interruption
|
Addressed the review feedback:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8843ff6bfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await asyncio.wait( | ||
| {self._done_fut, self._interrupt_fut}, | ||
| return_when=asyncio.FIRST_COMPLETED, |
There was a problem hiding this comment.
When a user turn interrupts current pipeline/TTS speech, _user_turn_completed_impl relies on await current_speech.interrupt() (agent_activity.py:2776) before copying the agent chat context. This FIRST_COMPLETED wait now returns as soon as _interrupt_fut is set, but _tts_task_impl only derives and inserts the synchronized partial assistant transcript afterward (agent_activity.py:3270-3337). The next LLM is therefore launched from a snapshot missing text the user actually heard, while that interrupted item can be inserted after the newer turn; retain a true finalization wait for this internal transition path.
Useful? React with 👍 / 👎.
| logger.warning(f"error in done_callback: {cb}", exc_info=e) | ||
|
|
||
| self._done_fut.add_done_callback(_on_done) | ||
| self._interrupt_fut.add_done_callback(_on_done) |
There was a problem hiding this comment.
When a SpeechHandle watched by RunResult is interrupted, this invokes _mark_done_if_needed before _done_fut or the linked speech tasks finish. RunResult._mark_done() then freezes the result using incomplete _error and _maybe_run_final_output state, and _item_added() rejects synchronized partial assistant messages produced during subsequent interruption cleanup. Consequently, an interrupted session.run() or remote run can resolve with missing events, trigger an erroneous structured-output retry, or report success before a late generation failure is recorded; completion consumers need a callback tied to actual _done_fut finalization rather than the interruption notification.
Useful? React with 👍 / 👎.
…while draining tasks on interruption
|
Fixed:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5565cde686
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| running_tasks = [t for t in self._tasks if not t.done() and t is not curr] | ||
| if running_tasks: | ||
| await asyncio.wait(running_tasks) |
There was a problem hiding this comment.
For session-generated handles, _create_speech_task appends the active generation task to _tasks (agent_activity.py:970-978), so an interrupted handle normally enters this branch rather than returning on _interrupt_fut. If that task is stuck in a provider or user async source—the scenario INTERRUPTION_TIMEOUT is intended to recover—this still blocks for five seconds; if the task delays or suppresses cancellation, it can remain blocked even after the timeout calls _mark_done(), because this asyncio.wait no longer observes _done_fut. Thus await handle and wait_for_playout() do not reliably gain the immediate interruption behavior asserted by the new tests, which only use handles with an empty _tasks list; keep task draining on a separate internal finalization path.
Useful? React with 👍 / 👎.
…hout draining tasks in wait_for_playout
|
Fixed:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1e56187cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # the response already landed, cancelling the future no longer reaches it | ||
| self._rt_session.interrupt() | ||
| return | ||
| speech_handle._add_interrupt_callback(self._rt_session.interrupt) |
There was a problem hiding this comment.
When VoiceAgent.say() uses a realtime model with supports_say, this callback is never registered because the text is not None branch returns above after directly awaiting _rt_session.say(text). If callers interrupt that SpeechHandle while the provider's say future is pending, the newly immediate wait_for_playout() returns but no provider interrupt is sent; the provider can continue generating or begin speaking until the internal task is cancelled by the five-second interruption timeout. Apply the same interrupt-aware cancellation around the realtime say() future.
Useful? React with 👍 / 👎.
|
Addressed review feedback: omitted |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…identity from logs
|
Addressed review feedback:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f87a58bb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not say_fut.done(): | ||
| say_fut.cancel() |
There was a problem hiding this comment.
When Phonic is used with a delayed say()—for example, while waiting for readiness or consuming an async iterable—cancelling say_fut only cancels its returned _pending_generate_reply_fut; Phonic's say() separately starts _generate_reply_task, and _send_say() continues to send_say() without checking that future, while interrupt() is a no-op before a generation exists. The handle can therefore finish as interrupted but stale text can still be sent and spoken during a later turn. Fresh evidence since the earlier comment is the current Phonic implementation in livekit-plugins/livekit-plugins-phonic/livekit/plugins/phonic/realtime/realtime_model.py, where lines 1021-1044 decouple the send task from the future and lines 1046-1069 send without a cancellation check.
Useful? React with 👍 / 👎.
|
Addressed review feedback:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7cdce04bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if f.cancelled() and self._generate_reply_task and not self._generate_reply_task.done(): | ||
| self._generate_reply_task.cancel() |
There was a problem hiding this comment.
When say() replaces an earlier pending call, cancelling the old future schedules this callback for the next event-loop iteration, but by then self._generate_reply_task points to the new call's task. The old callback therefore cancels the replacement task, so back-to-back say() calls drop the latest text and leave its future pending until the 10-second timeout; capture the task created for this specific future instead of looking up the mutable session field.
Useful? React with 👍 / 👎.
|
Addressed review feedback:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dea7b90fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @pytest.mark.asyncio | ||
| async def test_say_cancellation_cancels_pending_send_task() -> None: | ||
| model = RealtimeModel(api_key="fake") | ||
| sess = RealtimeSession(model) |
There was a problem hiding this comment.
Each RealtimeSession(model) immediately starts the phonic-realtime-session task (realtime_model.py:519), but none of these tests calls await sess.aclose(). Because every test yields with asyncio.sleep, that task begins opening a real provider connection and remains pending; the autouse fail_on_leaked_tasks fixture in tests/conftest.py:341-372 will therefore fail teardown, in addition to violating the hermetic unit category. Close each session in a finally block or fixture.
AGENTS.md reference: AGENTS.md:L40-L42
Useful? React with 👍 / 👎.
|
Addressed review feedback:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e78b9b7ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @pytest.mark.asyncio | ||
| async def test_say_cancellation_cancels_pending_send_task() -> None: | ||
| model = RealtimeModel(api_key="fake") | ||
| sess = RealtimeSession(model) |
There was a problem hiding this comment.
Each RealtimeSession(model) immediately schedules _main_task(), and the first asyncio.sleep() lets that task call the real Phonic SDK's connect().__aenter__(); assigning an AsyncMock to _socket does not mock that connection. These unit tests can therefore make external network requests with the fake key and become environment-dependent. Fresh evidence after the earlier cleanup comment is that the added aclose() runs only in finally, after the sleeps have already allowed the connection attempt; mock sess._client.conversations.connect or prevent _main_task from starting.
AGENTS.md reference: AGENTS.md:L40-L44
Useful? React with 👍 / 👎.
|
closing since |
2 participants
Summary
SpeechHandle.wait_for_playout(), wait for either_done_futor_interrupt_futusingasyncio.FIRST_COMPLETED.self.interruptedorself._interrupt_fut.done()is true instead of remaining deadlocked until the 5.0sINTERRUPTION_TIMEOUTexpires.tests/test_speech_handle_interruption.py.Fixes #5359