fix(cli): strip OpenRTC flags; align subcommands with LiveKit Agents CLI#20
Merged
mahimairaja merged 18 commits intomainfrom Mar 22, 2026
Merged
fix(cli): strip OpenRTC flags; align subcommands with LiveKit Agents CLI#20mahimairaja merged 18 commits intomainfrom
mahimairaja merged 18 commits intomainfrom
Conversation
openrtc start/dev forwarded --agents-dir, --dashboard, and other OpenRTC-only options in sys.argv to livekit.agents.cli.run_app, which uses a separate Typer app. That produced "No such option: --agents-dir" and immediate exit. Strip those flags while preserving forwarded LiveKit options (e.g. --reload). Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a versioned JSONL metrics stream, in-memory event buffering/draining, a Textual TUI sidecar that tails JSONL, expanded CLI commands/flags (LiveKit handoff, metrics JSONL options, TUI launch), scheduler refactor for independent dashboard/JSONL cadence, tests, and documentation/theme updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Pool as AgentPool
participant Store as RuntimeMetricsStore
participant Reporter as RuntimeReporter
participant Sink as JsonlMetricsSink
participant File as JSONL File
Note over Pool,File: Emitting metrics and events
Pool->>Store: record_session_started/finished/failed
Store->>Store: buffer event in deque
Reporter->>Store: drain_stream_events()
Store-->>Reporter: return & clear events
Reporter->>Sink: write_snapshot(snapshot)
Sink->>File: append snapshot envelope (rgba(0,128,0,0.5))
Reporter->>Sink: write_event(event)
Sink->>File: append event envelope (rgba(0,0,255,0.5))
sequenceDiagram
participant TUI as MetricsTuiApp
participant File as JSONL File
participant Parser as parse_metrics_jsonl_line
participant UI as UI Widgets
Note over TUI,UI: TUI polling and render flow
loop every 0.25s
TUI->>File: read new lines
TUI->>Parser: parse line
alt kind == "snapshot"
Parser->>UI: update status/agents/detail (rgba(255,165,0,0.5))
else kind == "event"
Parser->>UI: update event panel (rgba(128,0,128,0.5))
else malformed/unknown
Parser-->>TUI: ignore
end
end
TUI->>UI: user quits ('q')
TUI->>File: close handle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 |
|
|
Add console, connect, and download-files subcommands that delegate to the same LiveKit Typer app as python agent.py. Group help into OpenRTC vs Connection panels; expose --url, --api-key, --api-secret, and --log-level with the same env vars as LiveKit. Extend worker command tests accordingly. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
cursor
bot
changed the title
Add versioned JSON Lines export (--metrics-jsonl) with optional interval, JsonlMetricsSink, and RuntimeReporter integration. Truncate file on worker start. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Add optional tui extra and dev dependency on Textual. New command tails --metrics-jsonl with a small live layout. Add parse_metrics_jsonl_line for consumers and tests. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Queue session_started/finished/failed in RuntimeMetricsStore, drain into the JSONL stream after each snapshot. Extend parse_metrics_jsonl_line and the Textual TUI to show the last event. StubPool gains drain_metrics_stream_events. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
- CLI: start with --metrics-jsonl writes valid snapshot records; tui import guard asserted via caplog when openrtc.tui_app fails to import. - metrics_stream: reject unknown kind; JsonlMetricsSink requires open(); reporter ordering snapshot then drained events. - tui_app: event line rendering; malformed JSON line skipped before valid. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
- Root help + epilog: typical path is dev/start with only --agents-dir and LIVEKIT_* env; note conservative defaults and Advanced panel in subcommand help. - Group tuning flags under Rich panel "Advanced" (refresh interval, JSONL interval, default greeting, participant identity, log level, metrics JSON file, list --resources). - Slim download-files to agents-dir + connection only; provider defaults unused. - Test that download-files rejects --default-stt. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Update user-facing documentation to reflect the new `openrtc[tui]` extra, `openrtc tui`, `--metrics-jsonl`, console/connect/download-files commands, CLI ergonomics (help panels, flag stripping), and `drain_metrics_stream_events()`. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Add a custom theme extending the default with OpenRTC cyan/navy tokens, DM Sans + JetBrains Mono, glassy nav, hero gradient title, and layout tweaks for the landscape banner. Switch nav logo to logo.png, copy brand PNGs into docs/public, and rebuild the index as a home layout with features. Ignore node_modules for local docs builds. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Assert exit code 2 and the unknown flag name only; Click's default English phrase is gettext-translated when locale catalogs differ. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
… test Click's CliRunner only overrides env keys passed into invoke; a narrow COLUMNS from CI caused Rich to wrap the unknown flag so 'default-stt' was not a contiguous substring. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR stabilizes and modernizes the openrtc CLI by aligning worker subcommands with the LiveKit Agents CLI, stripping OpenRTC-only flags before delegating to LiveKit, and adding a versioned JSONL metrics stream plus an optional Textual TUI sidecar for tailing those metrics.
Changes:
- Delegate
start/dev/console/connect/download-filesto LiveKit with OpenRTC-only flags removed to prevent “unknown option” failures. - Add JSONL metrics stream (
--metrics-jsonl, interval control) with session lifecycle events, plusopenrtc tui --watch(Textual extra). - Update tests and documentation to cover the new CLI shape, metrics export, and docs site theming.
Reviewed changes
Copilot reviewed 20 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Adds locked dependencies for textual and markdown parsing deps via the new tui extra. |
pyproject.toml |
Introduces tui optional extra (textual) and adds it to the dev dependency group. |
src/openrtc/cli_app.py |
Adds flag stripping before LiveKit delegation, new subcommands, connection override flags, and JSONL reporting support. |
src/openrtc/metrics_stream.py |
New module defining the JSONL envelope schema, parsing, and a thread-safe JSONL sink. |
src/openrtc/tui_app.py |
New Textual app that tails the JSONL metrics output and renders snapshots/events. |
src/openrtc/resources.py |
Extends runtime metrics store with a bounded event deque and draining API for JSONL export. |
src/openrtc/pool.py |
Exposes AgentPool.drain_metrics_stream_events() to feed JSONL reporting. |
tests/test_cli.py |
Expands CLI coverage for new subcommands, flag stripping, JSONL output, and missing-Textual behavior. |
tests/test_metrics_stream.py |
Adds tests for JSONL schema parsing/sink behavior and RuntimeReporter JSONL emission. |
tests/test_tui_app.py |
Adds Textual-sidecar tests (skipped if textual isn’t installed). |
docs/index.md |
Converts docs landing page to a VitePress “home” layout with hero/features. |
docs/getting-started.md |
Documents installing cli vs cli,tui and adds a CLI quick path section. |
docs/cli.md |
Updates CLI reference for new subcommands, connection overrides, JSONL metrics, and TUI usage. |
docs/api/pool.md |
Documents runtime_snapshot() JSONL usage and adds drain_metrics_stream_events() API docs. |
docs/.vitepress/config.ts |
Adds font preconnect headers and switches logo asset to PNG. |
docs/.vitepress/theme/index.ts |
Adds a custom VitePress theme entry to load CSS. |
docs/.vitepress/theme/custom.css |
Adds brand-aligned VitePress styling and typography overrides. |
README.md |
Rewrites README to match the new CLI/TUI workflows and updated positioning. |
CONTRIBUTING.md |
Updates contributor install guidance for Textual / tui extra. |
AGENTS.md |
Updates contributor/test guidance and mentions the sidecar TUI workflow. |
.gitignore |
Ignores node_modules/ for the VitePress docs setup. |
.agents/skills/openrtc-python/SKILL.md |
Updates CLI examples and notes about OpenRTC-vs-LiveKit flag behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rtion Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai generate sequence diagram |
Sequence Diagram for PRsequenceDiagram
participant User
participant Client
participant API
participant Database
User->>Client: Initiates Request
Client->>API: Send Data
API->>Database: Query/Update
Database-->>API: Response
API-->>Client: Return Result
Client-->>User: Display Result
Note: The PR details (title, objectives, and walkthrough) were not provided in your request. Please share the specific PR information including:
This will allow me to generate a more accurate and detailed sequence diagram tailored to your specific use case. |
✅ Actions performedSequence diagram generated. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/openrtc/pool.py (1)
273-275: Prefer a typed event payload for this public API.Line 273 exposes
list[dict[str, Any]], which leaves the stream contract implicit for callers. Consider aTypedDict/dataclass event type and returnlist[MetricsStreamEvent]here.♻️ Proposed typing refinement
-from typing import Any, TypeVar +from typing import Any, TypeVar, TypedDict + +class MetricsStreamEvent(TypedDict, total=False): + event: str + timestamp: float + agent: str + error: str - def drain_metrics_stream_events(self) -> list[dict[str, Any]]: + def drain_metrics_stream_events(self) -> list[MetricsStreamEvent]: """Drain pending session lifecycle events for JSONL sidecar export.""" return self._runtime_state.metrics.drain_stream_events()As per coding guidelines, “Prefer concrete types over
Any” and “Prefer well-defined event payload types over loosely shaped dicts.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openrtc/pool.py` around lines 273 - 275, Change the public API of drain_metrics_stream_events to return a concrete event type instead of list[dict[str, Any]]: define a MetricsStreamEvent TypedDict or dataclass describing the expected keys/values, update the return annotation of drain_metrics_stream_events to list[MetricsStreamEvent], and propagate that type to runtime_state.metrics.drain_stream_events() (update its signature/implementation to return List[MetricsStreamEvent]). Ensure you add the necessary imports (TypedDict/dataclass and typing.List) and update any callers/tests to use the new MetricsStreamEvent type.tests/test_metrics_stream.py (1)
27-50: Consider extracting shared_minimal_snapshot()fixture.This helper is duplicated in
tests/test_tui_app.py. A sharedconftest.pyfixture would reduce duplication, though this is not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_metrics_stream.py` around lines 27 - 50, Duplicate helper _minimal_snapshot() returning PoolRuntimeSnapshot is used across tests; extract it into a shared pytest fixture in conftest.py. Create a fixture (e.g., minimal_snapshot or minimal_snapshot_factory) that constructs and returns the PoolRuntimeSnapshot (including ProcessResidentSetInfo and SavingsEstimate), replace local _minimal_snapshot() uses in tests with the injected fixture, and update imports/usage so tests call the fixture name instead of redefining _minimal_snapshot().src/openrtc/resources.py (1)
145-149: Minor: Redundantlist()wrapper.
raw_eventsis already a list fromstate.get("_stream_events", []), so thelist(raw_events)call on line 147 is redundant. It's harmless but could be simplified.♻️ Optional simplification
raw_events = state.get("_stream_events", []) self._stream_events = deque( - list(raw_events), + raw_events, maxlen=_STREAM_EVENTS_MAXLEN, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openrtc/resources.py` around lines 145 - 149, The code wraps raw_events in an unnecessary list when constructing self._stream_events; raw_events already comes from state.get("_stream_events", []), so remove the redundant list(...) and pass raw_events directly into deque(..., maxlen=_STREAM_EVENTS_MAXLEN) (update the deque construction around self._stream_events and raw_events accordingly).docs/.vitepress/theme/custom.css (1)
5-5: Optional: Stylelint prefers string notation for@import.The static analysis suggests using a bare string instead of
url()for the import. Both are valid CSS, but the string form is more concise.♻️ Optional style fix
-@import url('https://fonts.googleapis.com/css2?family=DM+Sans:ital,opsz,wght@0,9..40,400..700;1,9..40,500..700&family=JetBrains+Mono:wght@400;500&display=swap'); +@import 'https://fonts.googleapis.com/css2?family=DM+Sans:ital,opsz,wght@0,9..40,400..700;1,9..40,500..700&family=JetBrains+Mono:wght@400;500&display=swap';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/.vitepress/theme/custom.css` at line 5, Replace the `@import` rule in docs/.vitepress/theme/custom.css to use string notation instead of url()—locate the `@import` declaration (the line starting with "@import url(") and change it to the shorter string form (a quoted URL) so the import uses "@import 'https://fonts.googleapis.com/...';" which satisfies the stylelint suggestion.src/openrtc/metrics_stream.py (1)
80-83: Document truncation behavior in the docstring.The
open()method truncates the file (mode"w"), which is intentional per the class docstring but could surprise users expecting append behavior. Consider adding a note in the method docstring.📝 Suggested docstring enhancement
def open(self) -> None: - """Create parent dirs, truncate file, open for append.""" + """Create parent dirs, truncate file, and open for writing. + + The file is truncated on each worker start so the TUI sees fresh data. + """ self._path.parent.mkdir(parents=True, exist_ok=True) self._file = self._path.open("w", encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openrtc/metrics_stream.py` around lines 80 - 83, The open() method currently opens the file with mode "w" (self._path.open("w", encoding="utf-8")), which truncates the file; update the open() method docstring to explicitly state that it will truncate existing files (not append) and that this is intentional per the class behavior so callers are not surprised—mention the use of "w" mode and reference the open() method and self._path.open call in the docstring.src/openrtc/tui_app.py (1)
20-27: Consider usingTextIOWrapper | Noneinstead ofAnyfor_fh.The file handle is typed as
Anybut could use a more precise type likeTextIOWrapper | Nonefromtyping(orIO[str] | None). This improves IDE support and catches potential misuse.♻️ Suggested type refinement
+from typing import Any, TextIO + class MetricsTuiApp(App[None]): ... def __init__(self, watch_path: Path, *, from_start: bool = False) -> None: super().__init__() self._path = watch_path.resolve() self._from_start = from_start - self._fh: Any = None + self._fh: TextIO | None = None self._buf = ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openrtc/tui_app.py` around lines 20 - 27, Replace the broad Any type on the instance file handle with a precise text-stream type: update the _fh annotation in __init__ (and any other attribute or usage sites) from "Any" to "io.TextIOWrapper | None" or "typing.IO[str] | None"; add the required import (from io import TextIOWrapper or from typing import IO) and ensure any code that assigns or reads _fh is compatible (e.g., open(..., "r", encoding=...) returns TextIOWrapper/IO[str]) so IDE type checks and linting pick up correct usage of _fh and related operations like reads/writes.src/openrtc/cli_app.py (2)
389-393: Edge case: value arguments starting with--would be skipped.The check
not argv_tail[i].startswith("--")assumes values don't start with--. A path like--weird-dirwould be incorrectly treated as another flag. This is an unlikely edge case but worth noting.Consider documenting this limitation or using a more robust parsing approach if paths with
--prefixes are plausible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openrtc/cli_app.py` around lines 389 - 393, The CLI loop that handles flags in _OPENRTC_ONLY_FLAGS_WITH_VALUE incorrectly treats any next token beginning with "--" as another flag and skips it; update the logic in the parsing loop that advances i (the argv_tail index for arg) so that when arg is in _OPENRTC_ONLY_FLAGS_WITH_VALUE you always consume the next token as the flag's value regardless of whether it starts with "--" (or alternatively detect known flags from the flag set rather than using startswith), ensuring argv_tail[i] (the value) is not accidentally skipped; adjust the handling around arg, i, and argv_tail to consume the value deterministically and document the behavior in the parsing function comment.
118-173: Consider extracting duplicated scheduling logic.The dashboard and non-dashboard branches share nearly identical scheduling code (lines 134-152 vs 156-173). This could be extracted into a helper method, though the current implementation is functional and readable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openrtc/cli_app.py` around lines 118 - 173, The _run method contains duplicated scheduling loops for dashboard and non-dashboard modes; extract that shared timing/wait/dispatch logic into a helper method (e.g., _scheduling_loop or _run_scheduling_loop) that accepts an optional live renderable or a flag to call _build_dashboard_renderable and live.update when provided. Move the shared computations and checks that use _stop_event, _needs_periodic_file_or_ui, _jsonl_sink, _refresh_seconds, _jsonl_interval, _write_json_snapshot, and _emit_jsonl into that helper and replace both duplicated while loops in _run with a single call to the new helper (ensuring the dashboard branch still wraps Live and calls the helper with the live object so final live.update/_build_dashboard_renderable behavior is preserved).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Around line 33-35: The tui extra in pyproject.toml currently installs only
textual which causes openrtc.cli:main (see cli.py:main()) to exit if typer and
rich are missing; update the tui extra to also include "typer" and "rich" (or
alternatively update docs to require installing both the cli and tui extras) so
that running `openrtc tui --watch` does not fail—modify the tui list in
pyproject.toml to add the two packages or add a documentation note clarifying
that the cli extra is required.
In `@src/openrtc/tui_app.py`:
- Around line 90-97: The code calls float(wall) directly from
self._latest.get("wall_time_unix") and will TypeError if wall is None; update
the widget rendering in the block using self._latest (around the use of _latest
and the "wall_time_unix" key and status.update) to coerce a safe fallback before
formatting (e.g., set wall_val = float(wall) if wall is not None else 0.0 or use
payload timestamp) and then use wall_val in the formatted string passed to
status.update (keep the query_one("#status", Static) and status.update usage
intact).
---
Nitpick comments:
In `@docs/.vitepress/theme/custom.css`:
- Line 5: Replace the `@import` rule in docs/.vitepress/theme/custom.css to use
string notation instead of url()—locate the `@import` declaration (the line
starting with "@import url(") and change it to the shorter string form (a quoted
URL) so the import uses "@import 'https://fonts.googleapis.com/...';" which
satisfies the stylelint suggestion.
In `@src/openrtc/cli_app.py`:
- Around line 389-393: The CLI loop that handles flags in
_OPENRTC_ONLY_FLAGS_WITH_VALUE incorrectly treats any next token beginning with
"--" as another flag and skips it; update the logic in the parsing loop that
advances i (the argv_tail index for arg) so that when arg is in
_OPENRTC_ONLY_FLAGS_WITH_VALUE you always consume the next token as the flag's
value regardless of whether it starts with "--" (or alternatively detect known
flags from the flag set rather than using startswith), ensuring argv_tail[i]
(the value) is not accidentally skipped; adjust the handling around arg, i, and
argv_tail to consume the value deterministically and document the behavior in
the parsing function comment.
- Around line 118-173: The _run method contains duplicated scheduling loops for
dashboard and non-dashboard modes; extract that shared timing/wait/dispatch
logic into a helper method (e.g., _scheduling_loop or _run_scheduling_loop) that
accepts an optional live renderable or a flag to call
_build_dashboard_renderable and live.update when provided. Move the shared
computations and checks that use _stop_event, _needs_periodic_file_or_ui,
_jsonl_sink, _refresh_seconds, _jsonl_interval, _write_json_snapshot, and
_emit_jsonl into that helper and replace both duplicated while loops in _run
with a single call to the new helper (ensuring the dashboard branch still wraps
Live and calls the helper with the live object so final
live.update/_build_dashboard_renderable behavior is preserved).
In `@src/openrtc/metrics_stream.py`:
- Around line 80-83: The open() method currently opens the file with mode "w"
(self._path.open("w", encoding="utf-8")), which truncates the file; update the
open() method docstring to explicitly state that it will truncate existing files
(not append) and that this is intentional per the class behavior so callers are
not surprised—mention the use of "w" mode and reference the open() method and
self._path.open call in the docstring.
In `@src/openrtc/pool.py`:
- Around line 273-275: Change the public API of drain_metrics_stream_events to
return a concrete event type instead of list[dict[str, Any]]: define a
MetricsStreamEvent TypedDict or dataclass describing the expected keys/values,
update the return annotation of drain_metrics_stream_events to
list[MetricsStreamEvent], and propagate that type to
runtime_state.metrics.drain_stream_events() (update its signature/implementation
to return List[MetricsStreamEvent]). Ensure you add the necessary imports
(TypedDict/dataclass and typing.List) and update any callers/tests to use the
new MetricsStreamEvent type.
In `@src/openrtc/resources.py`:
- Around line 145-149: The code wraps raw_events in an unnecessary list when
constructing self._stream_events; raw_events already comes from
state.get("_stream_events", []), so remove the redundant list(...) and pass
raw_events directly into deque(..., maxlen=_STREAM_EVENTS_MAXLEN) (update the
deque construction around self._stream_events and raw_events accordingly).
In `@src/openrtc/tui_app.py`:
- Around line 20-27: Replace the broad Any type on the instance file handle with
a precise text-stream type: update the _fh annotation in __init__ (and any other
attribute or usage sites) from "Any" to "io.TextIOWrapper | None" or
"typing.IO[str] | None"; add the required import (from io import TextIOWrapper
or from typing import IO) and ensure any code that assigns or reads _fh is
compatible (e.g., open(..., "r", encoding=...) returns TextIOWrapper/IO[str]) so
IDE type checks and linting pick up correct usage of _fh and related operations
like reads/writes.
In `@tests/test_metrics_stream.py`:
- Around line 27-50: Duplicate helper _minimal_snapshot() returning
PoolRuntimeSnapshot is used across tests; extract it into a shared pytest
fixture in conftest.py. Create a fixture (e.g., minimal_snapshot or
minimal_snapshot_factory) that constructs and returns the PoolRuntimeSnapshot
(including ProcessResidentSetInfo and SavingsEstimate), replace local
_minimal_snapshot() uses in tests with the injected fixture, and update
imports/usage so tests call the fixture name instead of redefining
_minimal_snapshot().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68f85a1a-f9d3-43a8-ab2c-5d71365160ad
⛔ Files ignored due to path filters (3)
docs/public/banner.pngis excluded by!**/*.pngdocs/public/logo.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.agents/skills/openrtc-python/SKILL.md.gitignoreAGENTS.mdCONTRIBUTING.mdREADME.mddocs/.vitepress/config.tsdocs/.vitepress/theme/custom.cssdocs/.vitepress/theme/index.tsdocs/api/pool.mddocs/cli.mddocs/getting-started.mddocs/index.mdpyproject.tomlsrc/openrtc/cli_app.pysrc/openrtc/metrics_stream.pysrc/openrtc/pool.pysrc/openrtc/resources.pysrc/openrtc/tui_app.pytests/test_cli.pytests/test_metrics_stream.pytests/test_tui_app.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 24 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+390
to
+392
| i += 1 | ||
| if i < len(argv_tail) and not argv_tail[i].startswith("--"): | ||
| i += 1 |
There was a problem hiding this comment.
_strip_openrtc_only_flags_for_livekit() only skips the value token for OpenRTC flags if the next arg does not start with --. Click/Typer will happily accept values that begin with -/-- for options that take a value (e.g. a path like --tmp), so this can leak the value into the delegated LiveKit argv and break parsing. Since these flags are known to take exactly one value, it’s safer to always skip the next token (when present) rather than using the startswith('--') heuristic.
Suggested change
| i += 1 | |
| if i < len(argv_tail) and not argv_tail[i].startswith("--"): | |
| i += 1 | |
| # These flags always take exactly one value; skip the flag and its value. | |
| i += 2 |
src/openrtc/cli_app.py
Outdated
Comment on lines
+146
to
+152
| if self._needs_periodic_file_or_ui and now >= next_periodic: | ||
| live.update(self._build_dashboard_renderable()) | ||
| self._write_json_snapshot() | ||
| next_periodic += self._refresh_seconds | ||
| if self._jsonl_sink is not None and now >= next_jsonl: | ||
| self._emit_jsonl() | ||
| next_jsonl += self._jsonl_interval |
There was a problem hiding this comment.
RuntimeReporter._run() advances next_periodic/next_jsonl with += interval. If the loop ever falls behind by more than one interval (slow I/O, GC pause, terminal slowness), wait_* can become 0 repeatedly and the thread may busy-loop emitting multiple updates back-to-back to “catch up”. Consider setting next_* = now + interval (or advancing in a while now >= next_*: next_* += interval loop) to avoid tight loops and excessive writes under load.
src/openrtc/cli_app.py
Outdated
Comment on lines
+168
to
+173
| if self._needs_periodic_file_or_ui and now >= next_periodic: | ||
| self._write_json_snapshot() | ||
| next_periodic += self._refresh_seconds | ||
| if self._jsonl_sink is not None and now >= next_jsonl: | ||
| self._emit_jsonl() | ||
| next_jsonl += self._jsonl_interval |
There was a problem hiding this comment.
Same scheduling issue as above in the non-dashboard loop: using next_* += interval can produce a timeout=0 busy loop when the reporter falls behind. Adjusting next_* based on now (or skipping missed intervals) avoids CPU spikes and bursty disk writes.
Comment on lines
+40
to
+47
| def on_mount(self) -> None: | ||
| self._path.parent.mkdir(parents=True, exist_ok=True) | ||
| if not self._path.exists(): | ||
| self._path.touch() | ||
| self._fh = self._path.open("r", encoding="utf-8") | ||
| if not self._from_start: | ||
| self._fh.seek(0, 2) | ||
| self.set_interval(0.25, self._poll_file) |
There was a problem hiding this comment.
When tailing from EOF (from_start=False), the app seeks to the current end of file once and then only reads forward. If the worker restarts and truncates the JSONL file (which JsonlMetricsSink.open() does), the existing file handle’s position can remain past the new EOF and the TUI may never observe new writes. Consider detecting truncation (e.g. stat().st_size < fh.tell()) and seeking back to 0 (or reopening) in _poll_file().
src/openrtc/tui_app.py
Outdated
Comment on lines
+92
to
+94
| status = self.query_one("#status", Static) | ||
| status.update( | ||
| f"seq={seq} wall={float(wall):.3f} registered={payload.get('registered_agents')} " |
There was a problem hiding this comment.
_refresh_view() coerces wall_time_unix via float(wall) without verifying it’s present and numeric. Since parse_metrics_jsonl_line() currently doesn’t validate required fields/types, a record that passes the schema/kind checks but omits wall_time_unix (or sets it to a string) will raise and crash the TUI. Either validate required fields in parse_metrics_jsonl_line() or guard here with a safe default / type check.
Suggested change
| status = self.query_one("#status", Static) | |
| status.update( | |
| f"seq={seq} wall={float(wall):.3f} registered={payload.get('registered_agents')} " | |
| try: | |
| wall_str = f"{float(wall):.3f}" | |
| except (TypeError, ValueError): | |
| wall_str = "?" | |
| status = self.query_one("#status", Static) | |
| status.update( | |
| f"seq={seq} wall={wall_str} registered={payload.get('registered_agents')} " |
Comment on lines
+43
to
+57
| def parse_metrics_jsonl_line(line: str) -> dict[str, Any] | None: | ||
| """Return a parsed stream record (snapshot or event), or ``None`` if invalid.""" | ||
| stripped = line.strip() | ||
| if not stripped: | ||
| return None | ||
| try: | ||
| record: dict[str, Any] = json.loads(stripped) | ||
| except json.JSONDecodeError: | ||
| return None | ||
| if record.get("schema_version") != METRICS_STREAM_SCHEMA_VERSION: | ||
| return None | ||
| kind = record.get("kind") | ||
| if kind not in (KIND_SNAPSHOT, KIND_EVENT): | ||
| return None | ||
| return record |
There was a problem hiding this comment.
parse_metrics_jsonl_line() assumes json.loads() returns a dict, but JSONL input could be a list/number/etc. In that case this will raise at record.get(...) instead of returning None, which can crash the TUI/tail consumer on a single malformed line. Consider validating isinstance(record, dict) (and optionally required keys/types) before calling .get().
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/openrtc/tui_app.py (1)
90-97:⚠️ Potential issue | 🟡 MinorGuard against
Nonewall time to prevent crash on malformed records.If
wall_time_unixis missing orNonein a malformed record that passes schema validation,float(wall)will raiseTypeError. Add a fallback.🛡️ Suggested defensive fix
seq = self._latest.get("seq") wall = self._latest.get("wall_time_unix") status = self.query_one("#status", Static) + wall_val = float(wall) if wall is not None else 0.0 status.update( - f"seq={seq} wall={float(wall):.3f} registered={payload.get('registered_agents')} " + f"seq={seq} wall={wall_val:.3f} registered={payload.get('registered_agents')} " f"active={payload.get('active_sessions')} " f"uptime={float(payload.get('uptime_seconds', 0)):.1f}s" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openrtc/tui_app.py` around lines 90 - 97, The code calls float(wall) without guarding against wall being None, which raises TypeError for malformed records; in the block that reads seq = self._latest.get("seq") and wall = self._latest.get("wall_time_unix"), ensure wall is given a safe fallback (e.g., 0.0 or "n/a") before formatting, then use that safe value when building the status string passed to status.update in query_one("#status", Static); change the float conversion to operate on the fallback (or conditionally format as "n/a") so None cannot reach float().
🧹 Nitpick comments (1)
tests/test_metrics_stream.py (1)
188-251: Consider potential flakiness in timing-based tests.The tests use
time.sleep(0.6)andtime.sleep(0.85)with a 0.25s interval, expecting 2-3+ emissions. While the assertions use>= 1and>= 2to be lenient, on slow CI systems the actual number of emissions might vary.The current approach is reasonable for this use case since:
- The intervals are generous (2-3x the expected duration)
- Assertions check minimum counts rather than exact counts
- This tests actual time-based behavior that's hard to mock cleanly
If flakiness occurs, consider increasing sleep durations or using synchronization hooks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_metrics_stream.py` around lines 188 - 251, The timing-based tests test_runtime_reporter_emits_snapshot_then_drained_events_in_order and test_runtime_reporter_emits_jsonl_periodically can be flaky on slow CI because they rely on fixed time.sleep durations versus RuntimeReporter emissions (metrics_jsonl_interval and refresh_seconds); to fix, make the tests robust by either increasing the sleep durations (e.g., to cover additional intervals) or, better, replace the sleeps with a synchronization approach: expose or poll RuntimeReporter/Pool state (e.g., wait until path contains the expected number of lines or until seq increments) after reporter.start() and before reporter.stop(), using a short retry loop with a timeout so tests wait deterministically for emissions from RuntimeReporter rather than relying on fragile fixed sleeps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/.vitepress/theme/custom.css`:
- Line 5: The `@import` url(...) line violates the stylelint import-notation rule;
replace the url() form with a string literal import so the CSS uses `@import`
'https://fonts.googleapis.com/css2?family=DM+Sans:ital,opsz,wght@0,9..40,400..700;1,9..40,500..700&family=JetBrains+Mono:wght@400;500&display=swap';
(keep the same URL and trailing semicolon) to satisfy the lint rule and preserve
the font import.
---
Duplicate comments:
In `@src/openrtc/tui_app.py`:
- Around line 90-97: The code calls float(wall) without guarding against wall
being None, which raises TypeError for malformed records; in the block that
reads seq = self._latest.get("seq") and wall =
self._latest.get("wall_time_unix"), ensure wall is given a safe fallback (e.g.,
0.0 or "n/a") before formatting, then use that safe value when building the
status string passed to status.update in query_one("#status", Static); change
the float conversion to operate on the fallback (or conditionally format as
"n/a") so None cannot reach float().
---
Nitpick comments:
In `@tests/test_metrics_stream.py`:
- Around line 188-251: The timing-based tests
test_runtime_reporter_emits_snapshot_then_drained_events_in_order and
test_runtime_reporter_emits_jsonl_periodically can be flaky on slow CI because
they rely on fixed time.sleep durations versus RuntimeReporter emissions
(metrics_jsonl_interval and refresh_seconds); to fix, make the tests robust by
either increasing the sleep durations (e.g., to cover additional intervals) or,
better, replace the sleeps with a synchronization approach: expose or poll
RuntimeReporter/Pool state (e.g., wait until path contains the expected number
of lines or until seq increments) after reporter.start() and before
reporter.stop(), using a short retry loop with a timeout so tests wait
deterministically for emissions from RuntimeReporter rather than relying on
fragile fixed sleeps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ba170e6-08d8-48d1-87ca-1f05c19d1b17
⛔ Files ignored due to path filters (3)
docs/public/banner.pngis excluded by!**/*.pngdocs/public/logo.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.agents/skills/openrtc-python/SKILL.md.gitignoreAGENTS.mdCONTRIBUTING.mdREADME.mddocs/.vitepress/config.tsdocs/.vitepress/theme/custom.cssdocs/.vitepress/theme/index.tsdocs/api/pool.mddocs/cli.mddocs/getting-started.mddocs/index.mdpyproject.tomlsrc/openrtc/cli_app.pysrc/openrtc/metrics_stream.pysrc/openrtc/pool.pysrc/openrtc/resources.pysrc/openrtc/tui_app.pytests/test_cli.pytests/test_metrics_stream.pytests/test_tui_app.py
…ests - Include rich+typer in the tui extra so `openrtc tui` works without [cli]. - Harden TUI status line when wall_time_unix is missing/invalid; type _fh as TextIO. - Strip OpenRTC value flags by always consuming the following argv token. - Deduplicate RuntimeReporter scheduling loop; document JsonlMetricsSink truncation. - Add MetricsStreamEvent TypedDict and use it for drain_stream_events/drain_metrics. - Simplify deque unpickle for stream events; fix @import font notation for stylelint. - Share minimal_pool_runtime_snapshot fixture; poll JSONL in reporter tests. - Extend argv-strip test for values that look like flags. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
==========================================
+ Coverage 87.43% 90.10% +2.67%
==========================================
Files 5 7 +2
Lines 907 1274 +367
==========================================
+ Hits 793 1148 +355
- Misses 114 126 +12
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openrtc/cli_app.py (1)
926-1107:⚠️ Potential issue | 🔴 CriticalAdd context_settings to worker commands to allow LiveKit-only flags to pass through.
Typer subcommands reject unknown options by default. Without
context_settings={"allow_extra_args": True, "ignore_unknown_options": True}, flags like--reloadfail at parse time before_delegate_discovered_pool_to_livekit()can run. Apply the context settings to all worker commands (start,dev,console,connect) and add an end-to-end test confirming thatopenrtc dev --reloadreaches the downstream CLI.Suggested shape
+_LIVEKIT_PASSTHROUGH_CONTEXT = { + "allow_extra_args": True, + "ignore_unknown_options": True, +} + -@app.command("start") +@app.command("start", context_settings=_LIVEKIT_PASSTHROUGH_CONTEXT) def start_command( ... ) -> None: ... -@app.command("dev") +@app.command("dev", context_settings=_LIVEKIT_PASSTHROUGH_CONTEXT) def dev_command( ... ) -> None: ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openrtc/cli_app.py` around lines 926 - 1107, Update the Typer command decorators for the worker entrypoints to allow LiveKit-only flags to pass through: add context_settings={"allow_extra_args": True, "ignore_unknown_options": True} to the `@app.command` decorator for start_command, dev_command, console_command, and connect_command (so Typer won't reject flags like --reload before _delegate_discovered_pool_to_livekit or _run_connect_handoff run). Also add an end-to-end test that invokes the CLI (e.g., calls the top-level app with "dev --reload") and asserts the downstream handler was reached (mock or spy the _delegate_discovered_pool_to_livekit call) to confirm the --reload flag is forwarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openrtc/cli_app.py`:
- Around line 411-428: The helpers _apply_optional_livekit_connection_env and
_apply_optional_livekit_log_level currently mutate os.environ permanently;
change them to snapshot the original values for keys LIVEKIT_URL,
LIVEKIT_API_KEY, LIVEKIT_API_SECRET, and LIVEKIT_LOG_LEVEL before setting any
overrides and return a small context (e.g., a dict of previous values or a
context-manager-like object) so callers can restore the originals after the
LiveKit handoff; update callers that perform the handoff to use the
snapshot/restore (or a with-style context) so environment changes are limited to
the single invocation and are restored immediately after the handoff.
In `@src/openrtc/metrics_stream.py`:
- Around line 43-57: The parser currently accepts any JSON object with the right
schema_version and kind but doesn't validate required envelope fields; update
parse_metrics_jsonl_line to reject malformed envelopes by verifying that record
contains a numeric/integer "seq", a numeric "wall_time_unix" (int/float), and a
non-null "payload" (and if you expect a mapping for snapshots/events, ensure
payload is a dict), returning None when any of these checks fail; keep checks
after confirming record.get("schema_version") == METRICS_STREAM_SCHEMA_VERSION
and kind in (KIND_SNAPSHOT, KIND_EVENT) and use the same function name
parse_metrics_jsonl_line to locate and update the logic.
In `@src/openrtc/resources.py`:
- Around line 127-132: The deque _stream_events with default_factory lambda
using deque(maxlen=_STREAM_EVENTS_MAXLEN) silently drops oldest
MetricsStreamEvent entries when capacity is exceeded; change the buffering to
detect overflow instead of relying on maxlen by using a non-evicting container
(e.g., deque without maxlen or a list) and explicitly track drops: increment a
new overflow counter (e.g., metrics_overflow_count), emit a log via the existing
logger, and/or push a synthetic MetricsStreamEvent indicating "overflow" so
drain_stream_events() can include a record; update drain_stream_events() to emit
and reset the overflow counter when draining and ensure _STREAM_EVENTS_MAXLEN is
still enforced by rejecting new events (and updating the counter/log) once the
limit is reached; add a regression test that simulates a burst >
_STREAM_EVENTS_MAXLEN and asserts that the overflow counter/log/synthetic
overflow record is produced and no silent drops occur.
In `@src/openrtc/tui_app.py`:
- Around line 40-47: The TUI currently keeps one file handle opened in on_mount
(self._fh) and can miss new records if the JSONL stream is truncated or
replaced; update _poll_file to detect file shrink/rotation by comparing the
current file size and inode (or os.stat) against the handle's current offset and
stat, and when you detect truncation (size < current offset) or replacement
(different inode), close and reopen self._fh from self._path (or seek(0) for
truncation) and reset any read offsets (respecting self._from_start logic);
modify on_mount/_poll_file to handle reopen semantics robustly and add a
regression test that restarts/truncates the writer and asserts the TUI resumes
showing new records.
---
Outside diff comments:
In `@src/openrtc/cli_app.py`:
- Around line 926-1107: Update the Typer command decorators for the worker
entrypoints to allow LiveKit-only flags to pass through: add
context_settings={"allow_extra_args": True, "ignore_unknown_options": True} to
the `@app.command` decorator for start_command, dev_command, console_command, and
connect_command (so Typer won't reject flags like --reload before
_delegate_discovered_pool_to_livekit or _run_connect_handoff run). Also add an
end-to-end test that invokes the CLI (e.g., calls the top-level app with "dev
--reload") and asserts the downstream handler was reached (mock or spy the
_delegate_discovered_pool_to_livekit call) to confirm the --reload flag is
forwarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96bb5225-bfa7-42f2-9a58-e389e96ccd41
⛔ Files ignored due to path filters (3)
docs/public/banner.pngis excluded by!**/*.pngdocs/public/logo.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.agents/skills/openrtc-python/SKILL.md.gitignoreAGENTS.mdCONTRIBUTING.mdREADME.mddocs/.vitepress/config.tsdocs/.vitepress/theme/custom.cssdocs/.vitepress/theme/index.tsdocs/api/pool.mddocs/cli.mddocs/getting-started.mddocs/index.mdpyproject.tomlsrc/openrtc/cli_app.pysrc/openrtc/metrics_stream.pysrc/openrtc/pool.pysrc/openrtc/resources.pysrc/openrtc/tui_app.pytests/conftest.pytests/test_cli.pytests/test_metrics_stream.pytests/test_tui_app.py
…low, TUI reopen - Wrap LIVEKIT_* / LIVEKIT_LOG_LEVEL overrides in snapshot/restore around handoff. - Validate seq/wall_time_unix/payload in parse_metrics_jsonl_line; reject bool seq. - Replace maxlen deque drops with explicit cap, overflow counter, warning log, and synthetic metrics_stream_overflow row on drain. - Sync TUI file handle on inode change or truncation; read after reopen in same poll. - Allow unknown LiveKit flags on start/dev/console/connect via Typer context_settings. - Tests: reload passthrough, env restore, malformed envelope, overflow burst, TUI unlink. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
- Install openrtc[cli,tui] in GitHub Actions so Textual TUI tests run under cov. - Add JsonlMetricsSink.seq, bool wall_time, AgentPool.drain_metrics tests. - Expand tui_app tests for tail mode, reopen, bad payloads, wall fallback, run_metrics_tui, stat OSError, and defensive branches. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Exclude optional TUI module from Codecov project/patch math; pytest --cov still measures it in CI with [cli,tui]. Dots escaped for regex per Codecov docs. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
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
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.
Summary
openrtc dev/start/console/connect/download-filesdo not fail with unknown options.--metrics-jsonl) with snapshots and session lifecycle events;RuntimeReporterdual-timer loop for dashboard/file vs JSONL cadence.openrtc[tui]/openrtc tui --watchsidecar that tails the JSONL file (Textual).AgentPool.drain_metrics_stream_events()andRuntimeMetricsStorestream event deque with pickle-safe state.docs/cli.md, getting-started, index, pool API, CONTRIBUTING, AGENTS.md, SKILL.md updated for the above.Test plan
uv sync --group devuv run pytestuv run ruff check .anduv run ruff format --check .Summary by CodeRabbit
New Features
Documentation
Tests
Chores