fix(cli): TUI defaults and clearer --watch handling#23
Conversation
The TUI tails a JSONL file from --metrics-jsonl; passing an agents directory raised IsADirectoryError. Validate early, log a helpful message, and exit 1. Clarify --watch help text. 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 positional-argument support to OpenRTC CLI: agents directory and metrics JSONL paths can be supplied positionally. Implements argv-rewrite helpers, updates CLI defaults/help, adjusts TUI metrics defaults/validation, updates docs, and adds tests to cover the new behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
- Coverage 90.22% 90.15% -0.08%
==========================================
Files 13 13
Lines 1361 1422 +61
==========================================
+ Hits 1228 1282 +54
- Misses 133 140 +7
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Running `openrtc tui` with no flags now tails the conventional JSONL file in the cwd; pair the worker with `--metrics-jsonl ./openrtc-metrics.jsonl`. Document the constant in metrics_stream and align --metrics-jsonl help text. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
cursor
bot
changed the title
Update CLI reference, README, getting started, AGENTS, and skill examples to describe plain `openrtc tui`, the default watch file, and --watch overrides. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Rewrite dev|start|console ./agents [./metrics.jsonl] to --agents-dir and --metrics-jsonl before Typer parses (openrtc.cli_app.main). Add inject_worker_positional_paths helper and tests. Update --agents-dir help and docs. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
…d tui Generalize argv injection via inject_cli_positional_paths (inject_worker_positional_paths remains an alias). Apply preprocessing for all subcommands in main() sys.argv path. Document in README, docs/cli, getting-started, and AGENTS. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
…etrics Behavior unchanged: openrtc dev ./agents only sets --agents-dir. Improve --help, epilog, MetricsJsonlArg/--agents-dir text, README, and docs/cli.md; add inject test. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
|
@copilot review |
|
@mahimairaja I've opened a new pull request, #24, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR improves the OpenRTC CLI/TUI ergonomics by standardizing a default metrics JSONL filename, making openrtc tui tail that file by default, and adding a pre-parse rewrite that allows common path arguments to be passed positionally (instead of requiring --agents-dir, --metrics-jsonl, or --watch).
Changes:
- Introduces a shared default metrics JSONL filename (
openrtc-metrics.jsonl) and uses it as the default watch target foropenrtc tui. - Adds CLI argv rewriting (
inject_cli_positional_paths) so commands likeopenrtc dev ./agents [./metrics.jsonl],openrtc list ./agents, andopenrtc tui ./file.jsonlwork. - Updates help text and docs to reflect new defaults and positional path shortcuts; adds targeted tests for the new behaviors.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/openrtc/cli_app.py |
Sets TUI default watch path, improves TUI error handling, and applies positional-path argv rewriting in main(). |
src/openrtc/cli_livekit.py |
Implements positional-path rewriting helpers and exposes inject_cli_positional_paths. |
src/openrtc/cli_types.py |
Updates CLI option help to document positional usage and default TUI watch file. |
src/openrtc/metrics_stream.py |
Defines DEFAULT_METRICS_JSONL_FILENAME for consistent default naming. |
src/openrtc/tui_app.py |
Adds validation to reject directory watch paths and improves initial status to show the resolved watch path. |
tests/test_cli.py |
Adds tests for positional rewriting and for TUI default watch/help behavior. |
tests/test_tui_app.py |
Adds a unit test for rejecting a directory as the watch path. |
docs/getting-started.md |
Updates examples to use openrtc dev ./agents and openrtc tui default behavior. |
docs/cli.md |
Documents positional path shortcuts across commands and updated TUI workflow/examples. |
README.md |
Updates CLI/TUI usage examples to reflect positional paths and TUI default watch behavior. |
AGENTS.md |
Updates quick references and notes to match the new CLI/TUI defaults. |
.agents/skills/openrtc-python/SKILL.md |
Updates skill doc examples to use positional paths and the default TUI workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def validate_metrics_watch_path(path: Path) -> None: | ||
| """Ensure *path* can be used as the metrics JSONL file (not a directory).""" | ||
| resolved = path.resolve() | ||
| if resolved.exists() and resolved.is_dir(): | ||
| raise ValueError( | ||
| "'--watch' must be a JSONL file path (the same path you pass to " | ||
| "'--metrics-jsonl' on the OpenRTC worker). This value is a directory " | ||
| "— for example, use a file such as ./metrics.jsonl, not your agents " | ||
| f"folder. Got: {resolved}" | ||
| ) |
There was a problem hiding this comment.
validate_metrics_watch_path() raises an error that specifically says "'--watch' must be...". Since the TUI also supports supplying the watch path positionally (openrtc tui ./file.jsonl), the message can be misleading (user didn't necessarily pass --watch). Consider wording it in terms of the watch path/metrics file argument (optionally mentioning --watch) so the error matches both invocation forms.
src/openrtc/cli_app.py
Outdated
| cli.main( | ||
| args=inject_cli_positional_paths(list(argv)), |
There was a problem hiding this comment.
When main() is invoked programmatically with argv (non-None), Click/Typer will parse args=... but sys.argv is left unchanged. The LiveKit handoff (_livekit_sys_argv) relies on sys.argv to preserve forwarded LiveKit flags (e.g. --reload) and will otherwise drop them because sys.argv[1] != subcommand. Consider setting sys.argv = [prog_name, *injected_args] (and restoring it in finally, as you already do) so programmatic main(["dev", "./agents", "--reload"]) behaves like the real CLI invocation.
| cli.main( | |
| args=inject_cli_positional_paths(list(argv)), | |
| injected_args = inject_cli_positional_paths(list(argv)) | |
| # Mirror a real CLI invocation so LiveKit handoff logic that | |
| # inspects sys.argv sees the injected arguments (e.g. --reload). | |
| sys.argv = [previous_argv[0], *injected_args] | |
| cli.main( | |
| args=injected_args, |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@mahimairaja I've opened a new pull request, #25, to work on those changes. Once the pull request is ready, I'll request review from you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_cli.py (1)
688-707: Avoid asserting via privateMetricsTuiApp._path(Line 699).This test currently depends on
openrtc.tui_appinternals instead of CLI-facing behavior, which makes it brittle during harmless refactors.As per coding guidelines: "Test public behavior, not private implementation details."♻️ Suggested refactor (assert `run_metrics_tui` call contract instead of private state)
def test_tui_command_without_watch_uses_default_metrics_path( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: pytest.importorskip("textual") - import openrtc.tui_app as tu - from openrtc.tui_app import MetricsTuiApp + import openrtc.tui_app as tui_app_mod - seen: list[Path] = [] + seen: list[tuple[Path, bool]] = [] - def fake_run(self: MetricsTuiApp) -> None: - seen.append(self._path) + def fake_run_metrics_tui(path: Path, *, from_start: bool) -> None: + seen.append((path, from_start)) monkeypatch.chdir(tmp_path) - monkeypatch.setattr(tu.MetricsTuiApp, "run", fake_run) + monkeypatch.setattr(tui_app_mod, "run_metrics_tui", fake_run_metrics_tui) runner = CliRunner() result = runner.invoke(app, ["tui"], catch_exceptions=False) assert result.exit_code == 0 - assert len(seen) == 1 - assert seen[0] == (tmp_path / "openrtc-metrics.jsonl").resolve() + assert seen == [(Path("openrtc-metrics.jsonl"), False)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 688 - 707, The test is asserting a private attribute MetricsTuiApp._path; instead, monkeypatch the public entrypoint (e.g., openrtc.tui_app.run_metrics_tui or the module-level function that the "tui" CLI calls) to capture the path argument and assert that was called with (tmp_path / "openrtc-metrics.jsonl").resolve(); replace monkeypatch.setattr(tu.MetricsTuiApp, "run", fake_run) and seen.append(self._path) with monkeypatching the public function (e.g., monkeypatch.setattr(tu, "run_metrics_tui", fake_run_path)) where fake_run_path(path: Path) records the passed path, then assert the recorded path equals the expected resolved path and remove uses of the private _path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 688-707: The test is asserting a private attribute
MetricsTuiApp._path; instead, monkeypatch the public entrypoint (e.g.,
openrtc.tui_app.run_metrics_tui or the module-level function that the "tui" CLI
calls) to capture the path argument and assert that was called with (tmp_path /
"openrtc-metrics.jsonl").resolve(); replace
monkeypatch.setattr(tu.MetricsTuiApp, "run", fake_run) and
seen.append(self._path) with monkeypatching the public function (e.g.,
monkeypatch.setattr(tu, "run_metrics_tui", fake_run_path)) where
fake_run_path(path: Path) records the passed path, then assert the recorded path
equals the expected resolved path and remove uses of the private _path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 340a5d97-e37d-4f27-a135-35e486fee3a8
📒 Files selected for processing (12)
.agents/skills/openrtc-python/SKILL.mdAGENTS.mdREADME.mddocs/cli.mddocs/getting-started.mdsrc/openrtc/cli_app.pysrc/openrtc/cli_livekit.pysrc/openrtc/cli_types.pysrc/openrtc/metrics_stream.pysrc/openrtc/tui_app.pytests/test_cli.pytests/test_tui_app.py
…ys.argv for programmatic main(argv=...) Co-authored-by: mahimairaja <81288263+mahimairaja@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahimairaja/openrtc-python/sessions/8c419ba3-63a6-4ee9-9afc-eed8d22cd894
[WIP] Fix TUI defaults and clearer --watch handling
Summary
openrtc tuiwith no flags now tails./openrtc-metrics.jsonlin the current working directory (same as the default for--watch). Users can still pass--watch /path/to/file.jsonlfor a custom file.--metrics-jsonl ./openrtc-metrics.jsonlso the TUI and worker share one file. The filename is defined asDEFAULT_METRICS_JSONL_FILENAMEinmetrics_stream.py.--metrics-jsonlhelp mentions pairing with the default TUI file.--watchstill fails with a clear error (from the earlier commit on this branch).--watchvalues are not misleading.Tests
test_tui_help_documents_default_watch_pathtest_tui_command_without_watch_uses_default_metrics_path--watchstill works).Summary by CodeRabbit
New Features
openrtc dev ./agents ./openrtc-metrics.jsonl).openrtc tuitails./openrtc-metrics.jsonlby default when no path is provided.Documentation
Bug Fixes
Tests