You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Reviewed the selective preview-publish scoping (targets input → scope.ts → gated matrix/jobs/npm publish). The design is solid: scope.ts centralizes the group→build-target/npm-family mapping, releases are hard-rejected from using a partial targets value, and the out-of-scope-dependency pinning in bumpPackageJsons (version.ts:206-219) is a nice touch that keeps a rivetkit-only preview installable without republishing @rivetkit/engine-cli. I traced through all four target groups (including the container-runner-only case, where scopedFamilies correctly yields an empty family Set and publishAll/assertDiscoverySanity gracefully publish zero npm packages since container-runner has no npm family) and didn't find a correctness bug in the scoping logic itself.
A few smaller things worth a look:
.github/workflows/publish.yaml — Rust crate dry-run isn't scope-gated. "Dry-run Rust crate publish" (and the release "Publish Rust crates" step) run on every non-release trigger regardless of targets, so a container-runner-only or cli-only preview still pays for a cargo publish --dry-run of rivet-error-macros. Doesn't break anything, but it's slightly at odds with the PR's stated goal of skipping unrelated builds for a narrowed target — might be worth gating on contains(needs.context.outputs.targets, 'rivetkit') too (Rust crates are effectively part of the rivetkit family) if that step is non-trivial cost-wise.
Inconsistent if: gating style on the three "Place … binaries" steps.Place engine binaries in engine-cli platform packages, Place CLI binaries in CLI platform packages, and Place bundled engine in CLI platform packages have no if: condition at all, unlike their sibling Download … artifacts steps. They're safe today only because of the [ -e "$f" ] || continue guard against the unmatched glob, but it's a bit fragile/inconsistent with the rest of the file's explicit gating. Consider adding matching if: contains(needs.context.outputs.targets, '...') conditions for clarity.
contains() substring matching for scope gates is a bit fragile long-term. Several steps gate on contains(needs.context.outputs.targets, 'cli') / 'engine' / 'rivetkit' (plain substring match on a comma-joined string), whereas the build job's own gate uses a space-wrapped exact-match case pattern specifically to avoid substring false-positives. Today none of the four group names collide as substrings of each other, so it works, but it'd be safer/more consistent to reuse the same exact-match idiom everywhere these scope checks appear, so a future target name addition can't silently start matching the wrong steps.
Bootstrapping edge case in resolveLatestPublished (version.ts:155-167). If an out-of-scope dependency has never been published before (e.g., the very first run that narrows scope before a full/all-target run has ever published @rivetkit/engine-cli), npm view <dep> version returns nothing and the bump throws. Not a bug given this repo's packages are already published, but worth keeping in mind if a new target group/package family is introduced later — the first selective run excluding it will fail until at least one full run publishes it.
No test coverage for the new scope-resolution logic.parseTargetGroups, buildScope, packageFamily, scopedFamilies, the release partial-target rejection in resolveContext, and the dependency-pinning branch in bumpPackageJsons are all pure/easily-testable functions gating real production npm publishes, but have no unit tests. This matches the existing convention for scripts/publish (no test suite there today), so not a regression, but since this logic directly controls what gets published where, some coverage would help catch regressions before they hit a release.
Nothing here is blocking — mostly polish/hardening suggestions on top of what looks like a well-thought-out change.
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
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.
No description provided.