fix(release-kit): registry-driven versioning, explicit release commits, v7 action pins - #2990
Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
…umps explicitly npm version only performs git commit/tag when .git sits next to package.json; kits live in kits/<name>/ with .git at the repo root, so the release commit and tag were silently skipped and the branch kept the old version. Every run then recomputed the same version from package.json and re-runs collided with npm's 403 on previously published versions. Determine the next version from the union of package.json and all versions the registry has ever seen (npm view <pkg> time includes unpublished ones), bump with --no-git-tag-version, then commit and tag explicitly from the repo root using per-kit tag names (<pkg>@<version>). The commit fails loudly if the bump staged nothing, and the push happens only after a successful publish. Fixes #2978
Findings from adversarial review of the previous commit: - Stage npm-shrinkwrap.json in the release commit: npm version bumps it and all kits ship one, so the tag would not have reproduced the published tarball. - Add a workflow-level concurrency group: all releases push the same branch, and a concurrent run would fail its push after npm publish already succeeded. - Apply the bump level to the highest stable version instead of the overall highest. Previously any rc on the registry silently forced a prerelease bump (major+RC on a 1.3.0-rc line yielded 1.3.0-rc.N, not 2.0.0-rc.0), and a stable minor on a 2.0.0-rc line finalized 2.0.0. An rc line now continues only when the requested level does not open a higher line. - git push --atomic so the tag cannot land without the branch update. - set -o pipefail in the version step; run: shells default to bash -e without pipefail, so a failed npx semver was masked by tail. - Pin semver to 7.8.5.
c9353b1 to
dff59cf
Compare
Second adversarial review round: - Fail the version step when npm view exits non-zero with no output; previously || true mapped that to an empty taken-set and the run proceeded on package.json alone, 403ing late at publish. - Include time.unpublished.versions in the taken set and drop the literal unpublished key: a fully unpublished package keeps its reserved versions there, not as top-level time keys. - Skip gh release create when the release already exists, so a re-run after a transient gh failure does not mint a new npm version just to get a release page. - Refuse dispatch from a non-branch ref: the final push would create a stray branch named after the tag. - Document the depth-1 pending queue of the concurrency group: a third dispatch cancels the queued run, so dispatch kits one at a time. - Make the push step a literal block so a future second line cannot fold into the same command.
IzaakGough
left a comment
IzaakGough
left a comment
There was a problem hiding this comment.
Lgtm - Two things AI flagged (mostly just things to be aware of than fixes required afaik)
The time.unpublished.versions branch cannot be reached
npm view <pkg> <field> resolves the spec before reading the field, and a fully unpublished package has no versions to match, so it errors first and the parser exits at the E404 branch. Verified against left-pad-test, a real tombstone:
registry packument: time.unpublished.versions = ["0.0.1"]
npm view … time: {"error":{"code":"E404","summary":"Unpublished on 2017-01-13…"}}
current parser: (prints nothing, exits 0)
E404 makes "never published" and "fully unpublished" indistinguishable, so the reserved version the code was added to capture is dropped. The scenario needs a 72 hour window or npm support, so the risk is negligible either way; the reason to touch it is that three lines currently read as an active safeguard. Deleting them resolves it as well as fixing them does.
If fixing, reading the packument directly makes the branch reachable and drops the dependency on CLI output shape:
curl -sf "https://registry.npmjs.org/$PKG_NAME"Push after publish, for the record
Moving the push after publish is the better trade. The old ordering could leave a pushed commit and tag for a version that never reached npm, and since versions now come from the registry, npm running ahead of git is the recoverable direction.
The residual: if kits moves between the release job's checkout and its push, the push is rejected after publish succeeded, and --atomic takes the tag with the branch.
! [rejected] HEAD -> main (fetch first)
! [rejected] pkg@1.0.0 -> pkg@1.0.0 (atomic push failed)
The GitHub Release step is skipped too, so the version sits on npm with no commit, tag or release. The run goes red, and cleanup is a version bump, commit, tag and gh release create. The window is checkout to push, under a minute in the run logs, against roughly two pushes a day on kits. Optional insurance, if the manual cleanup is worth avoiding:
git fetch origin "$REF_NAME"
git rebase "origin/$REF_NAME"That does mean the tag points at a tree including kits/ changes that landed after the build, so it no longer matches the published tarball. Pushing the tag first on its own avoids that, since its name cannot race.
Optional: pin npm
npm 12 returns npm view … time --json as an array rather than an object, which the parser would read as one key "0", falling back to package.json alone. Not live today, since node-version: "24" gives npm 11.19.0, though Node does move npm majors mid-line (v18.14.0, v18.19.0, v20.7.0). npm i -g npm@11 settles it, and the curl above makes it moot.
npm view resolves the package spec before reading a field, so a fully unpublished package returns E404 and its reserved versions under time.unpublished never reach the parser; the unpublished handling added earlier was unreachable. npm view --json output shape also changes across npm majors (npm 12 returns an array), which would silently empty the taken-set. Fetching the packument from the registry avoids both: the registry 404s only for never-published names, and the document shape is the registry's, not the CLI's.
|
Nice catches, both reproduced exactly as you described: Fixed both in 28813dc by fetching the packument straight from the registry: HTTP_STATUS=$(curl -sS --retry 3 -o "$PACKUMENT" -w '%{http_code}' "$NPM_CONFIG_REGISTRY/$PKG_NAME")404 now means never-published only (tombstones return 200 with On the push race: agreed it's the better trade and we're leaving it as is. The fetch+rebase would make the tag point at a tree that no longer matches the published tarball, which is quieter but worse than a loud red run with a short manual cleanup. |
Syncs release-kit.yaml byte-for-byte with the version that landed on kits in #2990. This copy never executes (Release Kit is always dispatched from kits; next has no kits/ directories); it only lists the workflow in the Actions UI, and it had drifted to show the old broken versioning logic. No behavior change. Will need re-syncing if #2991 lands on kits.
2 participants
Fixes #2978.
<pkg>@<version>) from repo root. Commit fails loudly if nothing staged.npm view <pkg> time, includes unpublished versions) unioned with package.json.Action pin bumps for #2977 split out into #2991; the two merge independently (non-overlapping hunks).
Verified: dry-run dispatch green, computes 0.0.2-rc.1 for rtdb (the version #2978 requires); version logic passed 10 synthetic registry scenarios locally. The non-dry-run commit/tag/push path has not been exercised live.