ci: dry-run release workflows on PRs#1464
Merged
Merged
Conversation
Add pull_request triggers to the release-py, release-kt, release-swift, libmoq, and release-js workflows so packaging breakage is caught before a tag/main push, not after release-plz cuts a release and publish fails. Paths are scoped narrowly to the workflow yaml and the shell/TS scripts that actually drive packaging. Rust source changes under rs/moq-ffi and rs/libmoq stay out of the PR trigger; just ci already exercises them. Publish steps are gated behind github.event_name == 'push' so PR runs never touch PyPI, Maven Central, the Swift mirror, or npm: - release-py: derives version from Cargo.toml on PR (no tag); drops the now-unused PUBLISH_PYTHON repo-var guard - release-kt: swaps publishAndReleaseToMavenCentral for publishToMavenLocal - release-swift: gates release/publish; adds publish-dry-run job that runs publish.sh --dry-run - libmoq: gates the release job - release-js: adds DRY_RUN env var, plumbed through to npm publish js/common/release.ts grows a --dry-run flag (also picks up DRY_RUN=true env), forwards to npm publish --dry-run, and skips the already-published early-exit so the build + publish manifest is always exercised on PRs. release-rs.yml is left alone; release-plz has no usable dry-run, and just rs ci already runs cargo publish --dry-run on every crate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
npm publish --dry-run still hits the registry to check whether the version already exists, and errors out with "You cannot publish over the previously published versions" when it does. That's the common case on PRs of main, so every PR run was failing. npm pack --dry-run exercises the same tarball-build path (file list, shasum, integrity) without any registry roundtrip, which is all we need to validate on a PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Your plan currently allows 4 reviews/hour. Refill in 10 minutes and 28 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. WalkthroughThis PR enables dry-run testing of release workflows across Rust, JavaScript, Kotlin, Python, and Swift pipelines. Workflows now trigger on pull_request events with path filters, conditionally extract versions from Cargo.toml during PR runs while using existing release.sh helpers for pushes, gate production release/publish jobs to push events only, and propagate DRY_RUN behavior into JS publishing. The Swift workflow adds a publish-dry-run job to validate the publish flow in PRs without performing actual publishing. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/libmoq.yml:
- Around line 7-11: The pull_request.paths list in the libmoq workflow omits
rs/libmoq/Cargo.toml so the "Parse version" step is skipped for version-only
PRs; update the pull_request.paths array in .github/workflows/libmoq.yml to
include "rs/libmoq/Cargo.toml" alongside the existing entries (ensure the string
matches that exact path) so the workflow runs on PRs that only change the crate
manifest.
In @.github/workflows/release-kt.yml:
- Around line 7-12: The workflow's pull_request path filters are missing
rs/moq-ffi/Cargo.toml so PRs that only change that file won't trigger the
release-kt job; open .github/workflows/release-kt.yml and add
"rs/moq-ffi/Cargo.toml" to the pull_request.paths array (alongside existing
entries like "rs/moq-ffi/build.sh" and "kt/scripts/package.sh") so that the
parse-version step sees changes when github.event_name == 'pull_request'.
In @.github/workflows/release-py.yml:
- Around line 12-16: The pull_request paths list in the release workflow is
missing rs/moq-ffi/Cargo.toml so changes to that file won’t trigger the
workflow; update the pull_request.paths array in
.github/workflows/release-py.yml (the block under pull_request -> paths) to
include "rs/moq-ffi/Cargo.toml" alongside the existing entries so the
parse-version job will run when that file changes.
In @.github/workflows/release-swift.yml:
- Around line 7-13: The workflow's pull_request trigger omits
rs/moq-ffi/Cargo.toml while parse-version (used in the workflow) reads that
file, and publish-dry-run checkout step does not disable credential persistence;
update the pull_request.paths list to include "rs/moq-ffi/Cargo.toml" so PRs
touching that file trigger the workflow, and in the publish-dry-run job's
checkout step (symbol: publish-dry-run) add persist-credentials: false to match
the release job's checkout behavior (symbol: release) to avoid leaking tokens.
In `@js/common/release.ts`:
- Around line 14-17: Replace the shell-invoking execSync call that builds a
command string with a safe execFileSync call to avoid shell injection: where the
code sets published using execSync(`npm view ${name} version`, ...) (variable
published), call execFileSync("npm", ["view", name, "version"], ...) with the
same options; also audit other npm invocations in this module (e.g., the npm
pack --dry-run and npm publish --access public usages) and either switch them to
execFileSync("npm", [...]) equivalents or, per project guidance, replace them
with bun equivalents (bun pack / bun publish) if the repo mandates bun for JS/TS
tooling—ensure options/args and encoding/stdio behavior match the original
calls.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fab9233-0a67-4d06-9d81-f2f35e687f2b
📒 Files selected for processing (6)
.github/workflows/libmoq.yml.github/workflows/release-js.yml.github/workflows/release-kt.yml.github/workflows/release-py.yml.github/workflows/release-swift.ymljs/common/release.ts
- js/common/release.ts: switch all npm/bun invocations to execFileSync with an argv array. The npm view call previously interpolated the package name into a shell command. None of the strings here are attacker-controlled (package.json is from the repo), but execFileSync is the safer pattern and removes the ambiguity. - release-swift.yml: add persist-credentials: false to the publish-dry-run job's checkout, matching the release job. The clone uses anonymous HTTPS in dry-run mode so the persisted token wasn't used for anything; this is defense-in-depth hygiene. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/common/release.ts (1)
14-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix npm pre-publish version existence check
npm view <pkg> versioncompares against thelatestdist-tag, not whether the specific${name}@${version}already exists; the broadcatch {}can also mask auth/registry failures and incorrectly fall through tonpm publish.- Query
${name}@${version}and only swallow the not-found case.🔧 Suggested change
if (!dryRun) { let published = "0.0.0"; try { - published = execFileSync("npm", ["view", name, "version"], { + published = execFileSync("npm", ["view", `${name}@${version}`, "version"], { encoding: "utf8", stdio: ["pipe", "pipe", "pipe"], }).trim(); - } catch { - // Package not published yet + } catch (error) { + const stderr = String((error as { stderr?: unknown }).stderr ?? ""); + if (!stderr.includes("E404")) throw error; + // Version not published yet } if (version === published) { console.log(`⏭️ ${name}@${version} already published, skipping`); process.exit(0); } }🤖 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 `@js/common/release.ts` around lines 14 - 20, The code currently runs execFileSync("npm", ["view", name, "version"], ...) and swallows all exceptions, which mis-detects package existence and hides auth/registry errors; change the check to call execFileSync("npm", ["view", `${name}@${version}`, "version"], ...) (so you query the specific version) and only swallow the error when the command indicates the package@version is not found (e.g., parse the thrown error's stderr/message for the not-found marker), while rethrowing or surfacing any other errors (auth/registry/network) instead of falling through to publish; update the code paths around published, execFileSync, name, and version accordingly.
🤖 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.
Outside diff comments:
In `@js/common/release.ts`:
- Around line 14-20: The code currently runs execFileSync("npm", ["view", name,
"version"], ...) and swallows all exceptions, which mis-detects package
existence and hides auth/registry errors; change the check to call
execFileSync("npm", ["view", `${name}@${version}`, "version"], ...) (so you
query the specific version) and only swallow the error when the command
indicates the package@version is not found (e.g., parse the thrown error's
stderr/message for the not-found marker), while rethrowing or surfacing any
other errors (auth/registry/network) instead of falling through to publish;
update the code paths around published, execFileSync, name, and version
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73d0e99e-6369-4b1b-af5a-cafbd2c6072e
📒 Files selected for processing (2)
.github/workflows/release-swift.ymljs/common/release.ts
craneLib.cleanCargoSource's default filter only includes cargo-relevant extensions (.rs, .toml, .lock, etc.) and drops moq.pc.in. rs/libmoq/ build.rs reads moq.pc.in to generate target/pkgconfig/moq.pc, and the file-missing path is a silent `if let Ok(template) = fs::read_to_string` that just skips generation. The Nix installPhase then fails at `cp target/pkgconfig/moq.pc` with "No such file or directory". This bug landed in c8091bf ("Package moq-gst for release via Nix-built tarballs"). No libmoq tag has been cut since, so the Nix-based libmoq build path was never actually exercised in CI until this PR's dry-run. Fix: use lib.cleanSourceWith to compose the cargo source filter with a .pc.in passthrough. Verified locally with `nix build .#libmoq` — the resulting derivation now contains lib/pkgconfig/moq.pc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kixelated
pushed a commit
that referenced
this pull request
May 23, 2026
Brings in the dry-run + persist-credentials + Determinate-Nix opt-out changes from main (PRs #1463, #1464, #1465). Resolves the recurring conflict in moq-relay.yml by keeping this branch's Nix-based rewrite and layering the new patterns on top. Extends the dry-run pattern to the four workflows this branch adds plus moq-gst (which #1464 didn't cover): - moq-relay, moq-cli, moq-clock, moq-token-cli, moq-gst now trigger on pull_request when the workflow itself, its build script, the nix derivation, or the flake changes. The build job parses the version from Cargo.toml on PRs (release.sh parse-version expects a tag). The release job is gated on github.event_name == 'push'. - release-brew.yml gains a dry-run job that, on PRs touching the workflow or any formula template, renders every template against synthetic tarballs and runs ruby -c on the output. The publish job is gated to workflow_run events only. Also adds persist-credentials: false to all checkouts that don't push, and `with: { determinate: false }` to nix-installer-action, matching the main-branch convention. https://claude.ai/code/session_015J5tVAQ7ESjBhnzdeXfhgX
kixelated
pushed a commit
that referenced
this pull request
May 23, 2026
PR #1464 deliberately left moq-gst out of the dry-run set, and the reason is clear in practice: moq-gst's Nix build pulls the full gstreamer dep tree, so its macOS and Linux dry-runs both started failing under the cache contention this PR creates by triggering five release workflows in parallel. Revert moq-gst.yml to exactly the main-branch contents (push-only trigger, no concurrency block, no PR version fallback). The other four workflows (moq-relay, moq-cli, moq-clock, moq-token-cli) keep their dry-runs because their builds are smaller and aren't already on main. https://claude.ai/code/session_015J5tVAQ7ESjBhnzdeXfhgX
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
1 participant
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
Adds
pull_request:triggers to the release-py, release-kt, release-swift, libmoq, and release-js workflows so packaging breakage surfaces on PRs instead of after release-plz cuts a tag and the publish silently fails.What changed
rs/moq-ffi/**andrs/libmoq/**is deliberately excluded —just cialready exercises compilation on a single platform.github.event_name == 'push'so PR runs never touch PyPI, Maven Central, the Swift mirror, or npm:release-py.yml: derives version fromrs/moq-ffi/Cargo.tomlon PR; tag/Cargo verify skipped; the now-stalePUBLISH_PYTHONrepo-var guard is removed.release-kt.yml: swapspublishAndReleaseToMavenCentralforpublishToMavenLocalon PR.release-swift.yml: gatesrelease/publishjobs on push; adds apublish-dry-runjob that runsswift/scripts/publish.sh --dry-run(anonymous clone, no commit/push).libmoq.yml: gates thereleasejob on push.release-js.yml: passesDRY_RUN=trueenv on PR.js/common/release.tsgrows a--dry-runflag (also readsDRY_RUN=trueenv), forwards--dry-runtonpm publish, and skips the already-published early-exit so the build + publish manifest are always exercised on PRs.What's left alone
release-rs.yml— release-plz has no usable--dry-run, andjust rs cialready runscargo publish --dry-runon every workspace member.Follow-up
PUBLISH_PYTHONrepo variable can be deleted from repo settings (no longer referenced).Test plan
release.shis unchanged +release.tsis touched.release-py.ymlandrelease-swift.ymlandrelease-kt.ymlandlibmoq.ymlandrelease-js.ymlwere all modified, so each should fire its own PR dry-run on this PR (they trigger on their own yaml changing). Confirm each runs and skips its publish step.release-js: confirmnpm publish --dry-runran for every JS package.release-swift: confirmpublish-dry-runjob ran and reported the diff against the mirror.release-kt: confirmpublishToMavenLocalran instead of Maven Central publish.release-py: confirm wheels + sdist built across the full 5-target matrix and thepublishjob is skipped.libmoq: confirm all 5 targets built and thereleasejob is skipped.🤖 Generated with Claude Code