Post-merge cleanup: tests, fixtures, dist freshness check, publish=false fix, rust-version/edition outputs, 1.0.0#20
Merged
Conversation
The repository is owned by Sergey Vilgelm, not "SV Tools". Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
- Split index.js into a one-line entry and lib.js (the testable surface). ncc still bundles index.js as the action entry, but the logic now lives in a module that can be imported by tests. - Add node:test unit tests for parseMetadata covering the empty-workspace case, the missing-packages-key case, the matrix-vs-features cases, and every shape of the publish field. - Fix a real bug: cargo metadata represents `publish = false` as `[]`, not `false`. The previous `pkg.publish !== false` check classified unpublishable packages as publishable. Now: `null` and non-empty arrays are publishable; an empty array is not. - Default packages/publish/matrix outputs to `[]` so downstream `fromJson` never chokes on an empty string. - Drop dead `hasOwnProperty` guards (cargo metadata always emits both `publish` and `features`). - Drop @actions/github — it was a runtime dependency but never imported, and bundling its @octokit graph bloats dist/ for nothing. - Add `"type": "module"` to make the ESM entry explicit (silences the MODULE_TYPELESS_PACKAGE_JSON warning under node:test). - README: document the toolchain auto-install, the matrix-features behavior, and the `publish = false` exclusion. - Bump to 0.3.1. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
checks.yml: - Add `Dist` job that rebuilds and asserts `git diff --exit-code dist/`, so PRs with stale bundled output are caught up-front instead of being patched after-the-fact by the manual update-dist workflow. - Add `ToolchainPin` regression test: scaffolds a project with `rust-toolchain.toml` pinning a specific version and verifies the action installs the toolchain and reads metadata. Without this, the rustup-1.28 fix has no CI coverage. - Switch `npm install` to `npm ci` for deterministic, lockfile-faithful installs. - Add actions/setup-node for cached node + npm installs. - Run `npm test` in the Check job. update-dist.yml: - Use a per-run branch (`update-dist-<run_id>`) so back-to-back runs don't collide on a fixed branch name. - Open a PR via `gh pr create` instead of just pushing the branch. - Switch to `npm ci` for the same reasons as above. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
Four self-contained Rust fixtures under tests/fixtures/ exercise every code path of the action without depending on external repos: - single-public: one crate, default-publishable. - single-private: one crate with `publish = false` — exercises the publish=false fix from this branch. - workspace-mixed: workspace with three members — `app` (default), `private-lib` (publish = false), and `feature-lib` (default + foo + bar features). Exercises workspace iteration, the publish filter, and the per-feature matrix expansion together. - toolchain-pinned: one crate with `rust-toolchain.toml` pinning 1.85.0. Replaces the previous inline `ToolchainPin` job that scaffolded the project at runtime via heredoc. The new `Fixtures` job runs the action against each one and asserts exact outputs (with jq + sorted comparison for the workspace, since feature ordering is alphabetical and we don't want to hard-code that). For the toolchain-pinned case it also greps `rustup toolchain list` to confirm the install actually happened — a future regression that silently skipped ensureToolchain would fail this check. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
The action has been in active use and the API surface (inputs and outputs) is stable; mark 1.0.0. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Post-merge cleanup that hardens the action’s behavior and CI by fixing Cargo publish=false handling, adding test coverage + fixtures, and enforcing dist/ freshness in workflows.
Changes:
- Fixed publishability detection for Cargo metadata (
publish=[]forpublish=false) and added unit tests for allpublishshapes. - Refactored action code into
lib.js(testable) + thinindex.jsentry; expanded tests/fixtures to exercise real Cargo workspaces and toolchain pinning. - Hardened CI: deterministic
npm ci, addedDistjob to block staledist/, and updatedupdate-distautomation to open PRs per run.
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/parse-metadata.test.js | Adds node:test coverage for metadata parsing + publish semantics |
| tests/fixtures/workspace-mixed/** | Adds a mixed workspace fixture (private + featureful + app) |
| tests/fixtures/toolchain-pinned/** | Adds a pinned-toolchain fixture to verify rustup behavior |
| tests/fixtures/single-public/** | Adds a single publishable crate fixture |
| tests/fixtures/single-private/** | Adds a single non-publishable crate fixture |
| package.json | Adds tests to all, switches to ESM, adjusts fmt/check globs, bumps version, drops @actions/github |
| lib.js | Introduces testable core logic (toolchain install, cargo metadata, parsing, outputs) |
| index.js | Becomes a thin entrypoint delegating to lib.js |
| README.md | Documents toolchain install, matrix behavior, and publish=false exclusion |
| LICENSE | Updates copyright holder |
| .github/workflows/update-dist.yml | Uses setup-node + npm ci, opens PR via gh on per-run branch |
| .github/workflows/checks.yml | Adds Dist verification job and fixture-based regression coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SVilgelm
changed the title
The previous descriptions were vague (\"Raw Metadata\") and the examples used non-JSON pseudo-syntax that wouldn't actually parse. Replace them with descriptions that: - State that every output is a JSON-encoded string and reference `fromJson` for parsing. - Use real JSON in the examples (`["foo","bar"]` rather than `[foo, bar]`). - Document the `publish = false` exclusion and the restricted-registry inclusion under `publish`. - Note that `manifest-path` determines the directory the action runs from, and that any adjacent `rust-toolchain.toml` is honored via `rustup toolchain install` before metadata is read. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
Two new outputs derived from the per-package `rust_version` and
`edition` fields in cargo metadata:
- `rust-version`: workspace MSRV — the highest `rust-version` declared
by any package, compared numerically so `1.10` beats `1.9`. Empty
string when no package declares one. Direct use case: feed it into
`actions-rust-lang/setup-rust-toolchain`.
- `edition`: the highest edition used by any package in the workspace
(e.g. `2021`, `2024`). Empty string for an empty workspace.
Both are raw scalar strings (not JSON-encoded), so consumers can read
them directly with `${{ steps.x.outputs.rust-version }}` — no
`fromJson` needed.
Test coverage:
- 6 new unit tests covering the empty case, max-across-packages, the
numeric-vs-lexicographic edge case (1.10 > 1.9), and patch-level
versions.
- Fixtures updated: single-public declares 1.85/2021; workspace-mixed
declares 1.85/2021 in `app` and 1.90/2024 in `feature-lib` to
exercise the workspace-MSRV max logic. The `Fixtures` CI job now
asserts both new outputs for every fixture.
README and action.yml updated to document the outputs.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
SVilgelm
changed the title
Audit pass on comments added in this branch against the project's "only the WHY, never the WHAT, no rotting facts" guideline. - lib.js: shorten the rustup-1.28 toolchain comment from 5 lines to 3. The load-bearing fact is "the proxy doesn't auto-install"; the rest was paraphrasing what the next line of code does. - tests: drop the redundant `// Lexicographic compare would pick "1.9" over "1.10"` — the test name already says exactly that. - checks.yml: drop the Fixtures job header comment (mostly described what the job does, which is obvious from the name + steps). - checks.yml: drop the `# MSRV is max(1.85, 1.90)…` comment in the workspace-mixed step. It paraphrased the assertion just below it, and would silently rot if a fixture's version changed. Kept (good WHYs that aren't derivable from the code): the publish-field shape table, the compareVersion lexicographic warning, the editions int-compare note, the jq-sort rationale, and the ensureToolchain end-to-end verification grep. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
…dist
- lib.js: spawn cargo metadata with `cwd` set to the manifest's
directory and pass the absolute manifest path. Without this, rustup
would fall back to the default toolchain when a sub-directory
rust-toolchain.toml was the source of truth (rustup walks up, not
down). Metadata reading still worked, but ensureToolchain's install
and the actual cargo invocation could silently use different
toolchains.
- lib.js: sort features before emitting matrix entries. Cargo currently
uses a BTreeMap (so output was already alphabetical), but this isn't
contractual — make it explicit so consumers can rely on the order.
- lib.js: drop `{ required: true }` from `getInput("manifest-path")`
since action.yml declares `required: false` with a default. The
default makes the value always present, so the runtime check was
redundant and inconsistent with the published contract.
- update-dist.yml: switch `npm run all` → `npm run build` and
`git add .` → `git add dist/`. The bot is a dist/ rebuilder, not a
one-shot full-format-and-test agent; this keeps automated PRs
narrowly scoped and avoids accidentally bundling unrelated source
drift.
- tests: add a reverse-insertion-order feature test that would fail if
the new sort step were ever dropped.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
cargo accepts `publish = true` (emits as `publish: null`, same as omitting) but it isn't in the documented schema — the docs only cover `publish = false` and `publish = ["registry"]`. Dropping it keeps the fixture idiomatic; the fixture name conveys the intent. Verified: smoke test still emits `publish: ["single-public"]` after removal. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
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
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.
Follow-up to #19, addressing every item from the review and bumping to 1.0.0.
Summary
Code
publish = falsewas incorrectly classified as publishable. Cargo representspublish = falseas[]in metadata, notfalse. The previous checkpkg.publish !== falsealways returned true. Now:nulland non-empty arrays are publishable; an empty array is excluded.packages/publish/matrix. Previously, ifcargo metadataever returned without apackageskey, downstreamfromJson(...outputs.matrix)would crash on an empty string.@actions/github. It was a runtime dep but never imported; bundling its@octokit/*graph was bloatingdist/for nothing.lib.js(testable) + one-lineindex.jsentry. ncc still buildsindex.js→dist/index.js; tests import fromlib.jsdirectly.hasOwnPropertyguards (cargo metadata always emits bothpublishandfeatures).New outputs
rust-version— workspace MSRV: the highestrust-versiondeclared by any package, compared numerically (so1.10beats1.9). Empty string when no package declares one. Direct use case: feed intoactions-rust-lang/setup-rust-toolchain.edition— the highest edition used by any package (e.g.2021,2024). Empty string for an empty workspace.Both are raw scalar strings (not JSON-encoded), so consumers can read them directly with
${{ steps.x.outputs.rust-version }}— nofromJsonneeded.Tests
tests/parse-metadata.test.js) usingnode:test— no extra deps. 14 tests covering empty workspace, missingpackageskey, matrix-vs-features cases, everypublishshape, MSRV max-across-packages with the1.10 > 1.9numeric edge case, and edition max-across-packages.tests/fixtures/exercise every code path end-to-end without depending on external repos:single-public/— one crate, default-publishable,rust-version = "1.85",edition = "2021".single-private/— one crate withpublish = false, norust-version(regression test for the publish-false bug + MSRV-empty case).workspace-mixed/— workspace with three members:app(1.85 / 2021),private-lib(no MSRV,publish = false), andfeature-lib(1.90 / 2024 with default/foo/bar features). Exercises workspace iteration, the publish filter, per-feature matrix expansion, and the MSRV/edition max-across-packages logic together.toolchain-pinned/— one crate withrust-toolchain.tomlpinning1.85.0. Replaces the previous inlineToolchainPinjob that scaffolded a project at runtime.CI
Checkjob now also runsnpm test.Distjob rebuilds and assertsgit diff --exit-code dist/, so PRs with stale bundled output are blocked instead of being patched after-the-fact.Fixturesjob runs the action against each in-tree fixture and asserts exact outputs (now includingrust-versionandedition):jq-based sorted set comparison forworkspace-mixed(stable across cargo versions, since feature ordering is alphabetical).toolchain-pinned, also grepsrustup toolchain listto confirm the install actually ran end-to-end — a future regression that silently skippedensureToolchainwould fail this check.npm install→npm ciin both workflows for deterministic installs.actions/setup-nodeadded to both jobs for cached Node + npm installs (pinned by SHA, matching the existing convention).update-dist.ymlnow uses a per-run branch (update-dist-<run_id>) and opens a PR viagh pr create, instead of failing on the second run when the fixed branch already exists.Docs
publish = falseexclusion.Misc
Sergey Vilgelm(wasSV Tools).1.0.0— API (inputs/outputs) is stable, the action has been in active use, and the cleanup in this PR + fix: install pinned toolchain and harden cargo metadata IO #19 closes the major outstanding behavioral bugs.Breaking change to flag for the v1.0 cut
Packages with
publish = falseare now (correctly) excluded from thepublishoutput. Prior versions included them due to thepkg.publish !== falsebug. Any consumer that relied on the bug will see a behavior change.Test plan
npm run all— fmt + build + test all green locally; 14/14 unit tests pass.dist/index.jssmoke-tested against all four fixtures locally; outputs match expectations includingrust-versionandedition.Check,Dist,Test,Fixtures,Roas,Nuspecshould all pass.v1.0.0and move/refresh the floatingv1tag.