Conversation
|
|
…ssue templates, Makefile, .env.example Co-authored-by: mahimairaja <81288263+mahimairaja@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahimairaja/openrtc-python/sessions/8a5d0a46-51e0-4cf9-8e93-c595c640b3fe
Co-authored-by: mahimairaja <81288263+mahimairaja@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahimairaja/openrtc-python/sessions/54882aec-4789-4d45-a056-603f379337d6
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
+ Coverage 90.15% 90.32% +0.16%
==========================================
Files 13 13
Lines 1422 1426 +4
==========================================
+ Hits 1282 1288 +6
+ Misses 140 138 -2
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves developer experience and release automation by migrating CI to uv, introducing a deprecation warning for legacy session_kwargs turn-handling keys, and moving the changelog into the docs site with automated release-note prepending.
Changes:
- Emit a
DeprecationWarningwhen deprecated top-levelsession_kwargsturn-handling keys are used; update tests accordingly. - Migrate test/lint CI workflows to
uv(uv sync --group dev,uv run ...) and add a developer Makefile. - Add docs-native changelog (
docs/changelog.md) + VitePress sidebar link, plus release workflow logic to auto-prepend release notes.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/openrtc/pool.py |
Adds DeprecationWarning emission when legacy turn-handling keys are found in session_kwargs. |
tests/test_pool.py |
Adds tests asserting the warning is emitted for deprecated keys and not emitted for modern kwargs. |
tests/test_routing.py |
Updates routing/session tests to stop using deprecated turn-handling keys and to assert modern kwargs behavior. |
docs/changelog.md |
Introduces the docs-hosted changelog with an insertion marker for automated releases. |
docs/.vitepress/config.ts |
Adds Changelog link to the docs sidebar. |
.github/workflows/test.yml |
Migrates test CI to uv with lockfile caching. |
.github/workflows/lint.yml |
Migrates lint/typecheck CI to uv with lockfile caching. |
.github/workflows/publish.yml |
Builds/publishes via uv and adds changelog auto-prepend + commit/push to main. |
Makefile |
Adds uv-based convenience targets for common developer tasks. |
.github/ISSUE_TEMPLATE/bug_report.yml |
Adds bug report issue template. |
.github/ISSUE_TEMPLATE/feature_request.yml |
Adds feature request issue template. |
.env.example |
Documents supported environment variables for local/dev setups. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/publish.yml
Outdated
| @@ -20,23 +20,66 @@ jobs: | |||
| uses: actions/checkout@v4 | |||
| with: | |||
| fetch-depth: 0 | |||
| # Use a PAT so the changelog commit can trigger deploy-docs.yml. | |||
| # Falls back to GITHUB_TOKEN if the secret is absent (docs won't | |||
| # auto-deploy in that case, but the publish still succeeds). | |||
| token: ${{ secrets.CHANGELOG_PUSH_TOKEN || secrets.GITHUB_TOKEN }} | |||
|
|
|||
There was a problem hiding this comment.
actions/checkout on a release event checks out the tag commit (detached HEAD). The later git push origin HEAD:main can fail with a non-fast-forward if main has moved since the tag was created. Consider checking out main explicitly (e.g. ref: main) or git fetch origin main && git switch main && git pull --ff-only before modifying/committing docs/changelog.md so the push is reliably fast-forward.
.github/workflows/publish.yml
Outdated
| tag = os.environ["RELEASE_TAG"] | ||
| body = os.environ["RELEASE_BODY"].strip() | ||
| # published_at is ISO-8601; keep only the date part | ||
| date = os.environ["RELEASE_DATE"][:10] | ||
|
|
||
| new_section = f"## [{tag}] - {date}\n\n{body}\n\n---\n\n" | ||
|
|
There was a problem hiding this comment.
This job only runs for tags starting with v, so RELEASE_TAG will typically be like v0.0.16, but docs/changelog.md uses headings like ## [0.0.15] ... (no v). Consider normalizing the tag (e.g. stripping a leading v) before writing the new section to keep the changelog format consistent.
.github/workflows/publish.yml
Outdated
| new_section = f"## [{tag}] - {date}\n\n{body}\n\n---\n\n" | ||
|
|
||
| path = "docs/changelog.md" | ||
| with open(path) as f: | ||
| content = f.read() | ||
|
|
||
| marker = "<!-- releases -->" | ||
| if marker not in content: | ||
| raise SystemExit(f"Marker '{marker}' not found in {path}") | ||
|
|
||
| # Insert the new release section right after the marker line | ||
| content = content.replace( | ||
| marker + "\n", | ||
| marker + "\n\n" + new_section, | ||
| 1, | ||
| ) |
There was a problem hiding this comment.
The changelog update script is not idempotent: re-running the workflow for the same release will prepend the same section again. Consider checking whether a heading for this tag already exists in docs/changelog.md and skipping the update (and subsequent commit) if it does.
.github/workflows/publish.yml
Outdated
| RELEASE_DATE: ${{ github.event.release.published_at }} | ||
| run: | | ||
| python3 - <<'PYEOF' | ||
| import os, re |
There was a problem hiding this comment.
re is imported in the embedded Python script but never used. Removing it keeps the workflow script minimal and avoids unused-import lint noise if this script is later moved into the repository proper.
| import os, re | |
| import os |
Makefile
Outdated
| # All commands delegate to `uv run` so they pick up the locked dev environment. | ||
| # Run `uv sync --group dev` once to set up the environment, then use these targets. | ||
|
|
||
| .PHONY: help install test lint format typecheck dev clean |
There was a problem hiding this comment.
.PHONY doesn’t include the test-fast and format-check targets that are defined below. If files/directories with those names ever exist, make will treat them as up-to-date and skip the recipes. Add them to .PHONY to ensure the targets always run.
| .PHONY: help install test lint format typecheck dev clean | |
| .PHONY: help install test test-fast lint format format-check typecheck dev clean |
src/openrtc/pool.py
Outdated
| "session_kwargs={'turn_handling': {'endpointing': {...}, 'interruption': {...}}}. " | ||
| "See the AgentPool.add() docstring for the supported turn_handling structure.", | ||
| DeprecationWarning, | ||
| stacklevel=4, |
There was a problem hiding this comment.
stacklevel=4 likely points the warning location outside OpenRTC (e.g. into LiveKit/asyncio), since the call chain here is _extract_deprecated_turn_options → _build_session_kwargs → _run_universal_session. Using a smaller stacklevel (typically 3) will keep the warning anchored to an OpenRTC frame, which is more actionable for users.
| stacklevel=4, | |
| stacklevel=3, |
📝 WalkthroughWalkthroughThis PR adds project infrastructure improvements including environment setup templates, GitHub issue templates, developer convenience tooling (Makefile), CI/CD workflow modernization to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/publish.yml (1)
66-71: Minor: replacement will produce triple newlines before the previous release.The marker in
docs/changelog.mdis followed by a blank line (<!-- releases -->\n\n). This code replaces onlymarker + "\n", leaving the second newline in place, and inserts"\n\n" + new_section(which ends with---\n\n). The result is three consecutive newlines before the old release entry.Consider adjusting to preserve consistent spacing—though this is cosmetic and won't break anything.
Possible adjustment
- content = content.replace( - marker + "\n", - marker + "\n\n" + new_section, - 1, - ) + # Match marker + blank line to avoid extra whitespace + content = content.replace( + marker + "\n\n", + marker + "\n\n" + new_section, + 1, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml around lines 66 - 71, The replace call currently matches marker + "\n" and inserts "\n\n" + new_section, which leaves the original extra newline and yields three consecutive newlines; update the replacement to either match marker + "\n\n" or insert only "\n" before new_section so the final spacing is exactly two newlines; modify the content.replace invocation (the variables marker, new_section, and content.replace) to use the chosen approach so the changelog preserves consistent double-newline spacing before the previous release.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 8-10: Reorder the LIVEKIT environment variables so they are
alphabetically sorted to satisfy dotenv-linter: place LIVEKIT_API_KEY and
LIVEKIT_API_SECRET before LIVEKIT_URL (i.e., LIVEKIT_API_KEY,
LIVEKIT_API_SECRET, LIVEKIT_URL) in the .env example; update the block
containing LIVEKIT_URL, LIVEKIT_API_KEY, and LIVEKIT_API_SECRET accordingly to
remove the linter ordering warnings.
In @.github/workflows/publish.yml:
- Around line 51-56: The tag variable currently comes directly from RELEASE_TAG
and may include a leading "v", causing inconsistent changelog headings; update
the code that sets tag (the variable assigned from os.environ["RELEASE_TAG"]) to
remove a leading "v" (only if present) before constructing new_section so
new_section uses a bare semantic version (e.g., "0.0.16") instead of "v0.0.16".
In `@Makefile`:
- Line 5: Update the .PHONY declaration to include the missing targets so make
treats them as phony: add test-fast and format-check to the existing .PHONY line
(which currently lists help install test lint format typecheck dev clean) so the
Makefile's .PHONY includes test-fast and format-check.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 66-71: The replace call currently matches marker + "\n" and
inserts "\n\n" + new_section, which leaves the original extra newline and yields
three consecutive newlines; update the replacement to either match marker +
"\n\n" or insert only "\n" before new_section so the final spacing is exactly
two newlines; modify the content.replace invocation (the variables marker,
new_section, and content.replace) to use the chosen approach so the changelog
preserves consistent double-newline spacing before the previous release.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 798db0d8-111c-46ec-a45d-7113f5c82358
📒 Files selected for processing (12)
.env.example.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/workflows/lint.yml.github/workflows/publish.yml.github/workflows/test.ymlMakefiledocs/.vitepress/config.tsdocs/changelog.mdsrc/openrtc/pool.pytests/test_pool.pytests/test_routing.py
.env.example
Outdated
| LIVEKIT_URL=ws://localhost:7880 | ||
| LIVEKIT_API_KEY=devkey | ||
| LIVEKIT_API_SECRET=secret |
There was a problem hiding this comment.
Reorder LiveKit keys to satisfy dotenv-linter.
dotenv-linter flags these lines for key ordering. Reordering avoids CI lint noise/failures.
Suggested fix
-LIVEKIT_URL=ws://localhost:7880
LIVEKIT_API_KEY=devkey
LIVEKIT_API_SECRET=secret
+LIVEKIT_URL=ws://localhost:7880🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 9-9: [UnorderedKey] The LIVEKIT_API_KEY key should go before the LIVEKIT_URL key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The LIVEKIT_API_SECRET key should go before the LIVEKIT_URL key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 8 - 10, Reorder the LIVEKIT environment variables
so they are alphabetically sorted to satisfy dotenv-linter: place
LIVEKIT_API_KEY and LIVEKIT_API_SECRET before LIVEKIT_URL (i.e.,
LIVEKIT_API_KEY, LIVEKIT_API_SECRET, LIVEKIT_URL) in the .env example; update
the block containing LIVEKIT_URL, LIVEKIT_API_KEY, and LIVEKIT_API_SECRET
accordingly to remove the linter ordering warnings.
.github/workflows/publish.yml
Outdated
| tag = os.environ["RELEASE_TAG"] | ||
| body = os.environ["RELEASE_BODY"].strip() | ||
| # published_at is ISO-8601; keep only the date part | ||
| date = os.environ["RELEASE_DATE"][:10] | ||
|
|
||
| new_section = f"## [{tag}] - {date}\n\n{body}\n\n---\n\n" |
There was a problem hiding this comment.
Strip the v prefix from the tag to match existing changelog format.
Existing changelog entries use version numbers without the v prefix (e.g., ## [0.0.15] - 2026-03-22), but github.event.release.tag_name includes the prefix (e.g., v0.0.16). This will create inconsistent headings.
Proposed fix
tag = os.environ["RELEASE_TAG"]
+ # Strip leading 'v' to match existing changelog format (e.g., [0.0.15])
+ if tag.startswith("v"):
+ tag = tag[1:]
body = os.environ["RELEASE_BODY"].strip()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tag = os.environ["RELEASE_TAG"] | |
| body = os.environ["RELEASE_BODY"].strip() | |
| # published_at is ISO-8601; keep only the date part | |
| date = os.environ["RELEASE_DATE"][:10] | |
| new_section = f"## [{tag}] - {date}\n\n{body}\n\n---\n\n" | |
| tag = os.environ["RELEASE_TAG"] | |
| # Strip leading 'v' to match existing changelog format (e.g., [0.0.15]) | |
| if tag.startswith("v"): | |
| tag = tag[1:] | |
| body = os.environ["RELEASE_BODY"].strip() | |
| # published_at is ISO-8601; keep only the date part | |
| date = os.environ["RELEASE_DATE"][:10] | |
| new_section = f"## [{tag}] - {date}\n\n{body}\n\n---\n\n" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/publish.yml around lines 51 - 56, The tag variable
currently comes directly from RELEASE_TAG and may include a leading "v", causing
inconsistent changelog headings; update the code that sets tag (the variable
assigned from os.environ["RELEASE_TAG"]) to remove a leading "v" (only if
present) before constructing new_section so new_section uses a bare semantic
version (e.g., "0.0.16") instead of "v0.0.16".
Makefile
Outdated
| # All commands delegate to `uv run` so they pick up the locked dev environment. | ||
| # Run `uv sync --group dev` once to set up the environment, then use these targets. | ||
|
|
||
| .PHONY: help install test lint format typecheck dev clean |
There was a problem hiding this comment.
Add test-fast and format-check to .PHONY.
These targets are defined but missing from the .PHONY declaration, which could cause unexpected behavior if files with those names exist.
Proposed fix
-.PHONY: help install test lint format typecheck dev clean
+.PHONY: help install test test-fast lint format format-check typecheck dev clean📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .PHONY: help install test lint format typecheck dev clean | |
| .PHONY: help install test test-fast lint format format-check typecheck dev clean |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 5-5: Missing required phony target "all"
(minphony)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 5, Update the .PHONY declaration to include the missing
targets so make treats them as phony: add test-fast and format-check to the
existing .PHONY line (which currently lists help install test lint format
typecheck dev clean) so the Makefile's .PHONY includes test-fast and
format-check.
|
@copilot apply changes based on the comments in this thread |
…ol.py stacklevel) Co-authored-by: mahimairaja <81288263+mahimairaja@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahimairaja/openrtc-python/sessions/fe3f281d-4630-48bf-93c8-eef92e76c1a5
Applied all reviewer feedback in cc80a7c:
|
CHANGELOG.mdfrom repo rootdocs/changelog.mdwith the moved content plus an<!-- releases -->insertion markerdocs/.vitepress/config.ts)publish.ymlto auto-prepend a new changelog section from the GitHub release body on every release, then commit + push tomainpublish.yml: checkoutref: mainexplicitly to avoid detached-HEAD push failurespublish.yml: strip leadingvfrom release tag so headings stay## [0.0.16]not## [v0.0.16]publish.yml: idempotent – skip insert if section for tag already presentpublish.yml: remove unusedreimport; fix double-newline spacing (matchmarker + "\n\n")Makefile: addtest-fastandformat-checkto.PHONY.env.example: sort LIVEKIT vars alphabetically (KEY, SECRET, URL)pool.py: changestacklevel=4→stacklevel=3for more actionable DeprecationWarning location✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
New Features
Documentation
Chores
uvpackage manager with dependency cachingTests