Repository navigation
Add npm package distribution infrastructure - #5
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📜 Recent review details⏰ Context from checks skipped due to timeout of 18000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🧰 Additional context used📓 Path-based instructions (1).github/**/*.{yml,yaml}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2026-05-04T15:23:38.296ZApplied to files:
🪛 YAMLlint (1.38.0).github/workflows/release.yml[error] 132-132: too many spaces inside braces (braces) [error] 132-132: too many spaces inside braces (braces) [error] 135-135: too many spaces inside braces (braces) [error] 135-135: too many spaces inside braces (braces) [error] 138-138: too many spaces inside braces (braces) [error] 138-138: too many spaces inside braces (braces) 🔇 Additional comments (12)
📝 WalkthroughAdd npm package distribution infrastructure for prebuilt binaries Introduce a complete npm distribution and publishing pipeline so users can install What changed
Repository & build metadata updates
Behavioral notes
This delivers an end-to-end, tiered npm distribution flow—packaging, verification, WalkthroughAdds end-to-end npm prebuilt-binary distribution and publish pipeline: targets schema and matrix, facade package and runtime shims, TypeScript packager to assemble per-platform npm packages from release tarballs, publish tooling (Node + Bash), GitHub Actions workflows to build and publish artifacts, plus docs, CI wiring, and Rust/Cargo metadata changes. Changesnpm Prebuilt Binary Distribution & Publishing
sequenceDiagram Maintainer->>GH: push release tag Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CodeRabbit security review on #5 flagged two real issues in the new split-job npm-release workflow: - The publish job was iterating npm/dist/*/ and reading each package.json blindly. The artifact comes from the unprivileged build job which executes tag-supplied code, so a malicious tag could drop extra package directories that would then be published with NPM_TOKEN. Now publishes only a hardcoded allowlist (mirrors targets.json on master), verifies each package.json's `name` matches the expected scope/key, and fails the job if the artifact contains anything outside the allowlist. - For workflow_dispatch runs, RELEASE_SHA was set to github.sha (the workflow file's commit on the default branch) instead of the user-supplied tag. Manual backfill runs would check out master and build façade files from there around the tag's binaries. Now uses inputs.tag for dispatch (workflow_run path keeps head_sha).
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two further security findings from CodeRabbit's review of #5: - The publish loop trusted .version from the artifact-supplied package.json. Allowlist + name verification prevented publishing unexpected packages, but a malicious tag could still stamp an arbitrary new version onto an allowed package and have it published with NPM_TOKEN. Now derive EXPECTED_VERSION from the trigger tag (trusted metadata, recomputed in the publish job from workflow_run.head_branch / inputs.tag) and reject any package whose version doesn't match — strict string comparison, before npm view or npm publish. - --skip-missing was always passed to build-packages.mjs, so a release-triggered run could silently ship a half-empty matrix. Removing the flag entirely would break tier-3 BSDs (release.yml runs them with continue-on-error: true). Instead, enforce the safety property at the privileged publish step: split the allowlist into REQUIRED (tier 1/2) and OPTIONAL (tier 3 / experimental). Required missing fails the job; optional missing is skipped.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two more findings from CodeRabbit's review of #5: - build-packages.mjs always ran with --skip-missing, so a missing required (tier-1/2) target was deferred to the privileged publish job's required-vs-optional check rather than failing fast in the unprivileged build job. Make the script tier-aware: tier-3 experimental targets are silently skipped when their tarball is absent (release.yml runs them with continue-on-error: true), tier 1/2 fail the job. The workflow now only passes --skip-missing for manual workflow_dispatch backfills. - The publish loop's pre-check (npm view) is followed by npm publish with a TOCTOU window. If another actor publishes the same version between the two calls, npm exits with EPUBLISHCONFLICT and the job dies. Capture stderr, swallow the specific error, and treat it as a no-op — mirrors the logic in npm/scripts/publish.mjs.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two findings from CodeRabbit review against the npm package
generator and the standalone publish script:
- `build-packages.ts` only copied `lib/resolve.cjs` into the
façade dist tree, so the published `runner-run` shipped without
`lib/launch.cjs` even though both `bin/run.cjs` and
`bin/runner.cjs` `require("#launch")`. Every
`npm install runner-run && run` would have thrown
`MODULE_NOT_FOUND: '#launch'`. Mirror the existing bins loop
pattern so adding entries to the `imports` map is a single-list
edit going forward.
- `publish.ts` ran `npm view` / `npm publish` without an explicit
`--registry`, inheriting whatever was in ambient `.npmrc`. The
CI workflow's `npm.sh` already pins the registry and passes
`--ignore-scripts`, but `publish.ts` is its own invocation path
(its usage block advertises `node npm/scripts/publish.ts ...`).
Default `--registry` to `https://registry.npmjs.org/`,
overridable for testing, and add `--ignore-scripts` to the
publish call.
- `build-packages.ts`: swap `createReadStream().pipe(createGunzip())` for `readFile` + `promisify(gunzip)`. The piped form emitted read/decompress failures as `'error'` events that bypassed the caller's `try`/`catch`; the awaited buffered form rejects normally. Tarballs are small enough that buffering is fine. - `publish.ts`: mark `PathLike` as a `type`-only import. - `npm-release.yml`: drop the unused `WS` env on the `download release archives` step — the extracted script reads `GITHUB_WORKSPACE` directly.
- `release.yml`: new `matrix` job derives the rust build matrix from `npm/targets.json` so the inline 14-target list goes away. Adds `build-npm-dist` job after `upload-assets` that uploads `npm-dist` (retention 7d) for cross-workflow consumption. - `npm-release.yml`: drop the `build` job. Publish job now downloads `npm-dist` via cross-workflow `download-artifact` (`run-id` + `actions: read`) and sparse-checks-out `.github/scripts` from the default branch — no tag-supplied code on the filesystem at all. Adds required `run-id` `workflow_dispatch` input; `NODE_VERSION` set to `latest`. - `build-npm-packages.sh` / `npm.sh`: drop the `EVENT_NAME` / `--skip-missing` relaxation. The dist artifact is always produced in tag-push context where missing tier-1/2 tarballs already fail loud, so the optional-tier-1/2 publish branch was dead weight.
`targets.json` already declares a `netbsd-x64` entry with `"os": ["netbsd"]`, but the schema's `os` enum omitted it, so any schema-aware editor flagged the existing config as invalid.
- `Cargo.toml` / `Cargo.lock`: 0.5.0 → 0.6.0. - `npm/facade/package.json`: replace the `0.0.0-template` placeholder with `0.6.0` to track the crate version. The build still overwrites `tpl.version` at pack time, but valid SemVer in the source file stops looking like a stuck template to schema-aware tooling and humans skimming the repo. - `build-packages.ts` / `publish.ts`: add docstrings to the remaining undocumented functions (`errMessage`, `errCode`, `readOpts`, `readTargets`, `clean`, `buildFacade`, `buildPlatformPackage`, `main` / `parseAccess`, `main`). Pushes docstring coverage from 33% to 100% so CodeRabbit's 80% threshold stops failing on this PR.
- `Cargo.toml`: move authors into `[[package.metadata.authors]]`
(objects with `name`/`email`); add `[package.metadata.npm]` with
`name`/`subpkgscope`/`bugs`/`repository`/`engines`; rename feature
`run-alias` → `run`; bump `rust-version` to 1.97; declare
`build = "build.rs"`.
- `build.rs`: parse `package.metadata.authors[0]` and emit
`RUNNER_AUTHOR_NAME` + `RUNNER_AUTHOR_EMAIL` for `env!` /
`option_env!` consumers.
- `src/lib.rs`: replace clap's `crate_authors!()` (now empty since the
top-level `authors` field moved) with the env vars; drop the
`primary_author` / `authors` helpers; rename a `run-alias` test
fixture to match the feature.
- `npm/scripts/build-packages.ts`: call `cargo read-manifest` and
derive each per-platform `package.json` field
(`license`/`author`/`homepage`/`repository`/`bugs`/`engines`) from
Cargo metadata with per-field shape-narrowing. Refactor the tar
reader into named helpers and drop the hardcoded values.
- `npm/facade/lib/launch.cjs`: re-raise the child's terminating signal
on the facade itself so `set -e`, trap handlers, and Ctrl+C chains
see `128 + N` instead of an opaque `1`.
- `npm/facade/lib/resolve.cjs`: confirm the platform bin exists
before returning its path so a partial install fails loud here
instead of as an opaque `ENOENT` from `spawnSync` later.
- `npm/scripts/publish.ts`: only treat `ENOENT` as "missing" in
`exists()`; only treat `E404` as "not published" in
`alreadyPublished()` — auth/IO/network errors now bubble up.
- `npm/facade/package.json`: add `directories.{lib,bin}`.
- `justfile`: add dev recipes (`run`/`runner`/`build-packages`/
`test-release`/`sys`/`list`).
- `README.md` / `CHANGELOG.md`: rewrap; mark BSD targets experimental;
silence markdownlint duplicate-heading on the changelog.
- `build-packages.ts`: thread `meta` into `buildFacade` and spread it over the template (license, author, homepage, repository, bugs, engines) so the facade tracks `Cargo.toml` the same way the per-platform sub-packages already do since f33098d. Without this the facade's template values drift as soon as Cargo metadata changes. - `npm/facade/package.json`: drop the inline `engines.node` block. Cargo-derived `meta` now provides it; keeping a stale copy in the template just invited a silent override mismatch. - `publish.ts`: 120s timeout on `npm view` and `npm publish` subprocesses with an explicit signal-kill error. A stuck DNS or registry path used to be able to burn the full GitHub Actions 6h job budget; bound it to a fail-fast window so flakes retry quickly.
- `download-release-archives.sh`: `rm -rf npm/downloads` before recreating it. Stale `.tar.gz`/`.sha256` from a prior tag would pass `verify-checksum.sh` (which walks every file in the dir) but be wrong-version for the current `RELEASE_TAG`. Hosted GHA runners get fresh workspaces; self-hosted runners and local invocations don't. - `publish.ts`: add `validatePackageDir` and call it for every per- platform pkg and the façade before `alreadyPublished` / `publish`. Refuses to publish if the dist dir contains a `.npmrc`, if `package.json` declares `publishConfig`, or if `package.json#name` doesn't match the expected scoped/facade name. The `--registry` / `--access` CLI flags already override config-file values for the publish call itself, but rejecting the unexpected structure outright is cheaper than reasoning about precedence — and catches build-time mistakes where a sub-package gets scribbled into the wrong dir.
- `Cargo.toml`: drop `rust-version` 1.97 → 1.95. Hoist version-only
deps (`anyhow`, `colored`, `json5`, `serde_json`, `shlex`,
`yaml-rust2`) from per-dep `[dependencies.x]` tables into a single
`[dependencies]` block. Feature-bearing deps (`clap`,
`clap_complete`, `serde`) keep their own tables.
- `rust-toolchain.toml`: new — pin `channel = "1.95"` with `rustfmt`
+ `clippy` on `minimal` profile so cargo invocations match the
declared MSRV.
- `justfile`: collapse `default`/`run`/`runner` dispatch through a
private `shim` recipe that threads the build profile via `PROFILE`
env var; bin recipes now do a single `cargo run --bin=… --profile=…`
with `[arg(pattern=…)]` validation on the user-facing entrypoint.
Drop unused `targets` variable, the `sys` recipe, the stale
commented-out `npm-pkg`, and the cosmetic echo on `list`. Annotate
recipes with `[group('bins')]` / `[group('npm')]`.
- `build-packages` recipe: stop passing `--no-skip-missing` on the false branch. `build-packages.ts` already defaults `skip-missing` to `false`, so the explicit negation was just re-asserting the default and added a second code path for parseArgs to handle. - `test-release` recipe: pick `runner.exe`/`run.exe` on Windows and `runner`/`run` elsewhere, assert each binary exists in `target/release/` before `tar czf`, and feed the array into the tar invocation. Previously a missing/renamed bin would surface as a cryptic tar failure or, worse, a tarball missing the file with no warning.
`readTargets` previously erased `tier` from its return type. The main loop then silently skipped missing sub-package dirs and still shipped the facade — so a publish run on a `--only=<one-target>` build would push `runner-run` with truncated `optionalDependencies`, leaving every omitted required platform broken on `npm install`. Restore `tier` to the TS shape and add a pre-flight scan: if any tier-1/2 dir is absent under `npm/dist/`, throw with the missing names listed before any publish call fires. Tier-3 (FreeBSD arm64, NetBSD, OpenBSD) still passes through — those run with `continue-on-error` in the build matrix and the facade omits them from `optionalDependencies` intentionally.
a992f06 took `npm/scripts/*.ts` to 100%; this finishes the rest so CodeRabbit's 80% threshold stops flagging the remaining files on every PR: - `build.rs`: doc `main`. - `src/lib.rs`: doc `bin_name_from_arg0`, `configure_cli_command`, `help_byline`, `requests_version`. - `npm/scripts/build-packages.ts`: doc the remaining helpers (`isObject`, `narrow*`, `readCargoManifest`, `formatFirstAuthor`, `packageMetadata`, `errorMessage`/`errorCode`, `readOptions`, `parseOnlyList`, `readMatrix`, `cleanDist`, `buildFacade`, `buildPlatformPackage`, tar parsers, `formatPackage`, `main`). - `npm/scripts/publish.ts`: rewrite `parseAccess`, `readTargets`, `alreadyPublished`, `publish`, `main` docstrings. - `npm/facade/lib/resolve.cjs`: rewrite `resolveBinary` docstring. - `.github/scripts/publish/npm.sh`: add `publish_allowed` block comment. Drive-by: `justfile` `test-release` invokes the commands via `eval "${cmd}"` so quoted arguments in the array (`node …/run.cjs --version`) re-parse as proper shell tokens instead of word-splitting on whitespace as bare `${cmd}` expansion.
c17251b to
2dcb14d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- `bin_name_from_arg0`, `configure_cli_command`, `help_byline`, `requests_version`: make `pub` + `#[must_use]` and qualify the doctest examples with `runner::` so they actually compile against the crate. Previously private, so the ``` fences ran as doctests but referred to unreachable items. - Doc fences: ```text``` → ```bash``` for the CLI usage block, bare ``` → ```rust for the runnable examples. - Drop the `\[0\]` escapes in `argv[0]` references — rustdoc rendered the backslashes literally, and the brackets don't need escaping outside link syntax. - Reflow a couple of doc lines that drifted past column. `rust-toolchain.toml`: add `rust-analyzer` to the pinned 1.95 components so `cargo` and the editor agree on the toolchain.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Five fail-loud guards across the publish path; each catches a different bad input that previously fell through silently. - `derive-dist-dry.sh`: validate `INPUT_DIST_TAG` against `^[A-Za-z][A-Za-z0-9._-]*$` so a malformed override can't smuggle flag-like or whitespace values into `npm publish --tag`. Normalize `INPUT_DRY_RUN` to strict `true`/`false`; previously `True`/`1`/ `yes` all fell through the `[[ == "true" ]]` check as `dry_run=false`, i.e. a real publish under the appearance of a dry run. - `release.yml`: smoke-test the packaged `linux-x64-gnu` binary with `--version` before uploading the `npm/dist` artifact. Catches a broken bin at build time instead of as an `ENOENT` on every user's machine post-publish. Cross-target bins still rely on the publish job's install-time checks. - `npm.sh`: validate `optionalDependencies` in `publish_allowed`. The facade must list every required platform under the scope at exactly `EXPECTED_VERSION`; platform sub-packages must declare none. A tampered platform package could otherwise smuggle attacker-controlled deps that npm would happily install transitively. Wrap `npm view` and `npm publish` in `timeout 120s` with explicit 124 handling so a hung registry can't burn the full job budget. - `build-packages.ts`: `narrowRepository` / `narrowBugs` / `narrowAuthor` now throw when an optional field is present but wrong-typed, instead of silently dropping it. Cargo metadata drift (e.g. `repository.type` becomes a number) used to ship a facade with the field missing rather than failing the build.
Summary
This PR adds a complete npm distribution system for
runner-run, enabling users to install prebuilt binaries via npm without requiring a Rust toolchain. The implementation uses a façade package pattern with per-platform sub-packages distributed viaoptionalDependencies.Key Changes
Build system (
npm/scripts/build-packages.mjs): New script that generates npm package trees for the façade and all platform-specific sub-packages. Extracts native binaries from release tarballs and generates appropriatepackage.jsonfiles with platform constraints (os,cpu,libcfields).Publish system (
npm/scripts/publish.mjs): New script that publishes all generated packages to npm registry, publishing sub-packages first then the façade to ensure dependency resolution.Platform matrix (
npm/targets.json): Configuration file defining 14 supported platform targets across Linux (gnu/musl variants), macOS, Windows, FreeBSD, NetBSD, and OpenBSD with tier levels and build tool specifications.Façade package (
npm/facade/):package.json: Main entry point declaring all sub-packages asoptionalDependenciesbin/runner.jsandbin/run.js: Shim scripts that resolve and execute the correct platform-specific binary at runtimelib/resolve.js: Binary resolution logic that locates the installed sub-package and provides helpful error messages if none are foundREADME.md: User-facing documentation with installation instructions and troubleshooting guideCI/CD (
.github/workflows/npm-release.yml): New workflow that triggers after successful release builds, downloads release artifacts, verifies checksums, builds npm packages, and publishes to npm registry with optional dry-run and provenance support.Release workflow update (
.github/workflows/release.yml): Expanded build matrix from 2 to 14 targets with per-target runner specifications and build tools (cargo vs cross), added experimental flag support, and changed archive format to include all platforms.Documentation (
README.md): Added npm installation instructions and explanation of the façade pattern.Implementation Details
require.resolve(), avoiding postinstall scripts and network access during installation.--skip-missingflag for partial matrix builds.