Repository navigation
Nightly channel should release nightly - #4296
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe release pipeline now separates canary and nightly dispatches, creates source tags tied to exact commits, adds scheduled nightly orchestration, and computes Seattle-local nightly versions with strict tag and ordering validation. ChangesRelease pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
![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 (1)
.github/workflows/nightly-release.yml (1)
41-45: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope
actions: writeto the job that dispatches.Only
dispatch-nightlyneeds it;notify-slackalready narrows to reads. Moving it to the job level clears the zizmor finding and keeps the schedule-triggered token minimal.♻️ Proposed permission scoping
permissions: - # `actions: write` dispatches the release; the reads below (CI runs, release - # runs, commits, tags) need nothing beyond the default scopes. - actions: write contents: readThen in the
dispatch-nightlyjob:dispatch-nightly: name: Dispatch nightly release permissions: # `actions: write` dispatches the release; the reads (CI runs, release # runs, commits, tags) need nothing beyond the default scopes. actions: write contents: read🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/nightly-release.yml around lines 41 - 45, Move the actions: write and contents: read permissions from the workflow-level permissions block to the dispatch-nightly job. Keep both permissions on dispatch-nightly, while preserving notify-slack’s read-only permission scope and minimizing the schedule-triggered token.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 1378-1383: Update the canary-release gating logic around the
release.toml diff check to parse [release].canary_version from HEAD^ and HEAD,
and set cut=true only when those values differ. Keep the existing unchanged-path
output and canary version computation, while ignoring changes to other
release.toml fields such as nightly-version.
In `@scripts/baml-language-version`:
- Around line 358-374: Update the fallback handling in the version lookup flow
around command_output so an empty or tagless git ls-remote result is treated as
a failure, not returned as an empty tag list. Validate the parsed fallback tags
before returning and call fail with the existing collision-prevention message
when none are found; preserve normal filtering and successful non-empty results.
---
Nitpick comments:
In @.github/workflows/nightly-release.yml:
- Around line 41-45: Move the actions: write and contents: read permissions from
the workflow-level permissions block to the dispatch-nightly job. Keep both
permissions on dispatch-nightly, while preserving notify-slack’s read-only
permission scope and minimizing the schedule-triggered token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 43a5c63b-53d0-4103-9e79-ce86ffd06e0e
📒 Files selected for processing (9)
.github/workflows/ci.yaml.github/workflows/nightly-release.yml.github/workflows/release-baml-language.ymlbaml_language/sdks/agent-docs/bridge-ref/ref-java-packaging.mdbaml_language/sdks/java/baml_bridge/PUBLISHING.mdscripts/baml-language-versionscripts/tests/test_baml_language_version.pyscripts/tests/test_release_pipeline_contract.pytypescript2/app-website/app/changelog/changelog-list.tsx
Binary size checks passed✅ 7 passed
Generated by |
85bd569 to
67bdb5d
Compare
67bdb5d to
bdb1958
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
scripts/baml-language-version (1)
284-317: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueWrap the
ZoneInfolookup so a missing tz database fails legibly.The comment notes that only version computation needs the tz database, but if it is absent (bare Windows/Alpine Python without
tzdata),ZoneInfo(SEATTLE)raisesZoneInfoNotFoundErrorand the tool exits with a traceback instead of thefail()diagnostics every other error path in this script produces.♻️ Proposed change
- local_date = moment.astimezone(zoneinfo.ZoneInfo(SEATTLE)).date() + try: + seattle = zoneinfo.ZoneInfo(SEATTLE) + except zoneinfo.ZoneInfoNotFoundError: + fail( + f"the {SEATTLE} time zone is unavailable; install the `tzdata` " + "package so nightly versions can be named for the Seattle night" + ) + local_date = moment.astimezone(seattle).date()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/baml-language-version` around lines 284 - 317, Update night_for to catch zoneinfo.ZoneInfoNotFoundError when resolving SEATTLE and route it through fail() with a clear message explaining that the timezone database or tzdata package is required. Preserve the existing timezone-aware validation and date calculation behavior when the lookup succeeds..github/workflows/nightly-release.yml (1)
41-45: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueScope
actions: writeto the job that dispatches.
notify-slackalready declares its own narrower permissions, so the workflow-level grant only matters fordispatch-nightly. Moving it down satisfies least-privilege (and the zizmorexcessive-permissionsfinding) without behavior change.♻️ Proposed change
-permissions: - # `actions: write` dispatches the release; the reads below (CI runs, release - # runs, commits, tags) need nothing beyond the default scopes. - actions: write - contents: read +permissions: {}and on the job:
dispatch-nightly: permissions: # `actions: write` dispatches the release; the reads (CI runs, release # runs, commits, tags) need nothing beyond the default scopes. actions: write contents: read🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/nightly-release.yml around lines 41 - 45, Move the workflow-level permissions block from the root of the nightly release workflow into the dispatch-nightly job, preserving actions: write and contents: read there. Keep notify-slack’s existing narrower permissions unchanged and remove the redundant global grant.Source: Linters/SAST tools
scripts/tests/test_baml_language_version.py (1)
356-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTruncating
PATHto/usr/bin:/bincan break the interpreter lookup.The tool is invoked via its shebang, so
python3must be resolvable from thisPATH. On runners where Python comes from mise/asdf/a venv rather than/usr/bin, this test fails for reasons unrelated to tag lookup. Thebin/stubs already shadow the realgh/gitbecauseself.binis first, so the rest ofPATHdoesn't need dropping.♻️ Proposed change
- env["PATH"] = f"{self.bin}:/usr/bin:/bin" + # `self.bin` is first, so the stubs already shadow any real gh/git. + env["PATH"] = f"{self.bin}:{os.environ['PATH']}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_baml_language_version.py` around lines 356 - 371, Update the environment setup in the test using VERSION_TOOL so PATH preserves the existing interpreter-resolving entries while keeping self.bin first for the stub executables. Remove the hardcoded “/usr/bin:/bin” truncation and construct PATH by prepending self.bin to the existing PATH value.scripts/tests/test_release_pipeline_contract.py (1)
968-974: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
assertNotIn("FORCE", ...)passes only because the arm's prose uses lowercaseforce.The guard's intent is "
forcecannot reach the rollback arm", but the assertion is case-sensitive and the arm deliberately discussesforcein comments. Stripping comments before asserting makes the check express the intent instead of depending on comment casing.♻️ Proposed change
self.assertIsNotNone(rollback_arm, nightly_workflow) - self.assertNotIn("FORCE", rollback_arm.group(1)) + rollback_code = "\n".join( + line + for line in rollback_arm.group(1).splitlines() + if not line.lstrip().startswith("#") + ) + self.assertNotIn("force", rollback_code.lower())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_release_pipeline_contract.py` around lines 968 - 974, Update the rollback-arm assertion in the test around rollback_arm to remove comments from the matched shell text before checking for FORCE, while preserving the existing guard extraction and assertNotIn behavior. Ensure the check validates executable content rather than passing because comments use lowercase “force”.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/nightly-release.yml:
- Around line 41-45: Move the workflow-level permissions block from the root of
the nightly release workflow into the dispatch-nightly job, preserving actions:
write and contents: read there. Keep notify-slack’s existing narrower
permissions unchanged and remove the redundant global grant.
In `@scripts/baml-language-version`:
- Around line 284-317: Update night_for to catch zoneinfo.ZoneInfoNotFoundError
when resolving SEATTLE and route it through fail() with a clear message
explaining that the timezone database or tzdata package is required. Preserve
the existing timezone-aware validation and date calculation behavior when the
lookup succeeds.
In `@scripts/tests/test_baml_language_version.py`:
- Around line 356-371: Update the environment setup in the test using
VERSION_TOOL so PATH preserves the existing interpreter-resolving entries while
keeping self.bin first for the stub executables. Remove the hardcoded
“/usr/bin:/bin” truncation and construct PATH by prepending self.bin to the
existing PATH value.
In `@scripts/tests/test_release_pipeline_contract.py`:
- Around line 968-974: Update the rollback-arm assertion in the test around
rollback_arm to remove comments from the matched shell text before checking for
FORCE, while preserving the existing guard extraction and assertNotIn behavior.
Ensure the check validates executable content rather than passing because
comments use lowercase “force”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff3bd9ea-e2c9-4725-9c37-974af1d449f9
📒 Files selected for processing (8)
.github/workflows/ci.yaml.github/workflows/nightly-release.yml.github/workflows/release-baml-language.ymlbaml_language/sdks/agent-docs/bridge-ref/ref-java-packaging.mdbaml_language/sdks/java/baml_bridge/PUBLISHING.mdscripts/baml-language-versionscripts/tests/test_baml_language_version.pyscripts/tests/test_release_pipeline_contract.py
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/sdks/java/baml_bridge/PUBLISHING.md
Summary by CodeRabbit
New Features
Bug Fixes
Documentation