-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix initial capture_run rendering in text mode #4529
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
tinalenguyen
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.
just tested, it works!
breaks for agents that do not speak first
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes introduce initial run result capture and cross-thread exit signaling mechanisms to the text-mode console session. The console now manages synchronization of initial agent events through futures and provides coordinated shutdown via threading events, with corresponding integration in the agent session startup flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent Session
participant Console as Agents Console
participant UI as UI/Text Mode
participant Events as Event Processor
Agent->>Console: _set_initial_run(run_state)
Console->>Console: Store initial_run_fut
Agent->>Events: Process initial events
Events->>Console: RunEvent list ready
Console->>UI: run_coroutine_threadsafe(await RunResult)
UI->>Console: Display initial events
Note over Console: Check should_exit at<br/>each loop iteration
UI->>Console: User input / Keyboard interrupt
alt Exit Signaled
UI->>Console: signal_exit()
Console->>Console: Set _exit_flag
Console->>Agent: Propagate exit state
else Continue
UI->>Events: Process ongoing events
end
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
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@livekit-agents/livekit/agents/cli/cli.py`:
- Around line 1089-1121: The current loop awaiting
c._io_initial_run_fut/_await_initial_run via cf can block forever if RunResult
never completes; add a bounded wait fallback: record a configurable max wait
(e.g. MAX_INIT_WAIT), poll cf.result(timeout=short_poll) up to that limit, and
if exceeded cancel cf, call cf.result() inside contextlib.suppress to drain, set
c._io_initial_run_fut = None, then process any available events (or proceed
without events) by calling _print_run_event only for events returned if any, and
ensure CancelledError/TimeoutError/Exception are properly suppressed and
translated into continuing startup rather than hanging (affecting symbols:
initial_run_fut, _await_initial_run, cf, c._io_initial_run_fut,
_print_run_event, _ExitCli).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
livekit-agents/livekit/agents/cli/cli.pylivekit-agents/livekit/agents/voice/agent_session.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format code with ruff
Run ruff linter and auto-fix issues
Run mypy type checker in strict mode
Maintain line length of 100 characters maximum
Ensure Python 3.9+ compatibility
Use Google-style docstrings
Files:
livekit-agents/livekit/agents/cli/cli.pylivekit-agents/livekit/agents/voice/agent_session.py
🧬 Code graph analysis (2)
livekit-agents/livekit/agents/cli/cli.py (1)
livekit-agents/livekit/agents/voice/run_result.py (1)
RunResult(70-214)
livekit-agents/livekit/agents/voice/agent_session.py (1)
livekit-agents/livekit/agents/cli/cli.py (1)
_set_initial_run(391-393)
🔇 Additional comments (10)
livekit-agents/livekit/agents/cli/cli.py (8)
45-45: LGTM: RunEvent/RunResult import is appropriate for the new text-mode flow.
294-295: LGTM: initializing_io_initial_run_futand_exit_flagis clean and scoped.
327-330: LGTM: creating the initial run future only in text mode aligns with intended behavior.
391-400: LGTM: thread-safe initial run handoff and exit signaling look solid.
1122-1124: LGTM: early-exit guard in the main text-mode loop is a good safety net.
1133-1134: LGTM: treating KeyboardInterrupt as a clean CLI exit is consistent.
1167-1171: LGTM: exit check during “Thinking...” prevents stuck UI on shutdown.
1410-1415: LGTM: exit signaling in the signal handler ensures coordinated shutdown.livekit-agents/livekit/agents/voice/agent_session.py (2)
628-631: LGTM: text-mode capture gate is straightforward and matches the CLI hook.
715-721: LGTM: handing off initial run state to the console and gating the await is sensible.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| initial_run_fut = c._io_initial_run_fut | ||
| if initial_run_fut is not None: | ||
|
|
||
| async def _await_initial_run() -> list[RunEvent]: | ||
| run_result = await initial_run_fut | ||
| await run_result | ||
| return run_result.events.copy() | ||
|
|
||
| cf = asyncio.run_coroutine_threadsafe(_await_initial_run(), c.io_loop) | ||
| try: | ||
| with live_status(c.console, Text.from_markup(" [dim]Initializing...[/dim]")): | ||
| while True: | ||
| if c.should_exit: | ||
| cf.cancel() | ||
| with contextlib.suppress(Exception): | ||
| cf.result() | ||
| raise _ExitCli() | ||
| try: | ||
| events = cf.result(timeout=0.1) | ||
| c._io_initial_run_fut = None | ||
| for event in events: | ||
| _print_run_event(c, event) | ||
| break | ||
| except TimeoutError: | ||
| continue | ||
| except KeyboardInterrupt: | ||
| cf.cancel() | ||
| with contextlib.suppress(Exception): | ||
| cf.result() | ||
| raise _ExitCli() from None | ||
| except Exception: | ||
| pass | ||
|
|
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.
Potential hang if the initial run never completes.
If the agent doesn’t emit any initial speech/events, RunResult may never complete, leaving the console stuck in “Initializing...” and blocking user input. Consider a timeout/fallback to proceed with whatever events are available.
💡 Suggested guard to avoid indefinite blocking
async def _await_initial_run() -> list[RunEvent]:
run_result = await initial_run_fut
- await run_result
+ try:
+ await asyncio.wait_for(run_result, timeout=5.0)
+ except asyncio.TimeoutError:
+ # Proceed even if the initial run doesn't finish (e.g., no initial output).
+ pass
return run_result.events.copy()🤖 Prompt for AI Agents
In `@livekit-agents/livekit/agents/cli/cli.py` around lines 1089 - 1121, The
current loop awaiting c._io_initial_run_fut/_await_initial_run via cf can block
forever if RunResult never completes; add a bounded wait fallback: record a
configurable max wait (e.g. MAX_INIT_WAIT), poll cf.result(timeout=short_poll)
up to that limit, and if exceeded cancel cf, call cf.result() inside
contextlib.suppress to drain, set c._io_initial_run_fut = None, then process any
available events (or proceed without events) by calling _print_run_event only
for events returned if any, and ensure CancelledError/TimeoutError/Exception are
properly suppressed and translated into continuing startup rather than hanging
(affecting symbols: initial_run_fut, _await_initial_run, cf,
c._io_initial_run_fut, _print_run_event, _ExitCli).
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.