Repository navigation
build(version): fail the release when the tag and package.json disagree - #120
Merged
Merged
Conversation
`package.json`'s version is what electron-builder stamps into every artifact name and into the updater feed, and what `app.getVersion()` reports - so it is what the About panel shows. The git tag has to agree with it, and nothing in CI noticed when it did not: `package.json` sat at 1.2.2 while the newest release was v1.3.x, and only a human reading both numbers could tell. - `scripts/check-version.mjs` is the guard, with two modes because the two situations have genuinely different invariants. A tag build (`release.yml`) requires the tag to equal `package.json` exactly - that is the release blocker. A pull request (`verify.yml`) requires `package.json` only to not be *behind* the newest tag: equality cannot be required there, because a version bump is merged before the tag is pushed and `main` is legitimately ahead during that window. Requiring it would fail every unrelated PR opened in between. - It runs in a new `guard` job that `create-draft` needs, so a mismatched tag fails before any of it: no platform build, and no orphan draft release left behind for a tag that was wrong. - `verify.yml` runs it before `npm ci` (it reads only `package.json` and the tags, so a drifted version fails in seconds) and now fetches tags: the default depth-1 checkout fetches none, so the comparison would otherwise have had nothing to compare and silently passed. - A tag carrying the right version but without the `v` prefix gets its own message, because that is fatal rather than cosmetic: `release.yml` triggers on `tags: v*.*.*`, so such a tag would push nothing and publish no artifacts. - `CONTRIBUTING.md` documents the flow, including why the two modes differ. It recommends `npm version` (one command bumps, commits and tags, so the two cannot drift) while noting that a hand cut release, which is how earlier releases here were actually tagged, is fine as long as the invariant holds. - The About panel already read the real version (`app.getVersion()` over IPC, with no hardcoded string), so that acceptance item needed no code change. Verification: `npm run check:version` in every mode (matching tag, mismatched tag, unprefixed tag, and `package.json` behind the newest tag, the last induced with a temporary local tag); `test/versionGuard.test.ts` (11 new tests: version parsing, numeric vs lexicographic ordering, pre-release ordering, tag selection, and the script's real exit codes through `spawnSync`); `npm run typecheck`; `npm test` (248 pass); `npm run build`; `npm run check:design`; `npm run lint` (0 errors, the pre-existing warning baseline unchanged); `npx prettier --check` on every touched file. Not verified: the workflow changes cannot be executed locally. `verify.yml` runs on this PR, but `release.yml`'s guard job only runs on a tag push, so that path stays unexercised until the next release. It runs the same script as the PR path, and the tag-mode branch is covered by the unit tests. Closes #61
`verify.yml` used `fetch-tags: true`, which does not do what it sounds like: the
step reported "no version tag reachable, nothing to compare" and exited 0. The
guard went green while comparing nothing, which is the one failure mode a guard
must not have - and it is exactly the trap this PR was written to close.
actions/checkout only fetches every tag when it fetches all history. In the
shallow path it builds the refspec with `fetchTags`, which adds just the
checked-out ref's own tag, and a pull request ref (`refs/pull/N/merge`) has no
tag. `fetch-depth: 0` fetches all branches and tags; the repository is 184 commits
and 15 tags, so the full fetch is not worth optimising away.
The silent pass is closed too, so it cannot regress unnoticed: outside CI a
missing tag list is still tolerated, but under GITHUB_ACTIONS it now fails and
names the fix. Same call as `allowMissingDependencies: false` in
electron-builder.yml - a check that cannot run must not look like one that ran.
Covered by a regression test that runs the script in a directory with no git
repository at all.
Also fixes a Windows-only test failure found by CI: `spawnSync('npm', ...)`
returns `status: null` there, because npm is an `npm.cmd` shim that spawn cannot
execute without `shell: true`. The entry point is now asserted by reading
`package.json`, which is platform-independent, and CI runs the real
`npm run check:version` step regardless.
The regression test for the silent-pass hole asserted 'outside CI a missing tag list is tolerated' while passing `cwd` but not `env`. CI sets GITHUB_ACTIONS, so the child inherited it, took the CI branch and exited 1 - the test failed on all three platforms by asserting the opposite branch from the one it ran. The local case now runs with GITHUB_ACTIONS removed from the environment, and the whole file passes both normally and with GITHUB_ACTIONS=true set, which is the condition that broke it.
`compareVersions` compared prereleases with `<` on the raw strings, which is not
SemVer. `'beta.10' < 'beta.2'` is true lexicographically, so the guard reported
1.4.0-beta.10 < 1.4.0-beta.2
when the opposite is correct. That flows straight into `newestVersionTag`, so the
guard could select the wrong tag as the newest release and then validate
`package.json` against it. This project has shipped `v1.0.5-beta.1`, so the shape
is real rather than theoretical, and the old tests only covered `alpha < beta` and
pre-release < final, which is why they did not catch it.
`comparePrerelease` now implements SemVer 11.4: identifiers are compared left to
right, numeric identifiers numerically, numeric identifiers always rank below
alphanumeric ones, and a shorter set of identifiers loses once every shared one is
equal. No dependency is added for it.
Also tightens bare version tags, which were accepted as releases while
CONTRIBUTING requires `v<version>` and release.yml only listens for `v*.*.*`. A
pushed `2.0.0` therefore published nothing, and `verify.yml` nevertheless counted
it as the newest release - so every pull request failed with "package.json is
behind 2.0.0" over a version that never shipped. Only `v`-prefixed tags count as
release tags now, and a bare version tag is reported instead of ignored, since a
tag that silently produces no release is exactly the drift this guard exists to
surface. Tag mode also says so when a bare tag is both misnamed and mismatched.
The "newest release" wording becomes "newest version tag": the check reads
`git tag --list`, not GitHub Releases.
Tests: the six precedence cases that motivated this (`beta.2 < beta.10`,
`alpha < beta`, `1 < alpha`, `beta < beta.1`, `beta.1 < beta.2`,
`beta.2 < beta.10`) plus the deeper comparisons, `newestVersionTag` picking
`v1.4.0-beta.10` over `v1.4.0-beta.2`, `bareVersionTags`, and an end-to-end case
that builds a real throwaway git repository with a bare `2.0.0` tag and asserts it
is reported rather than compared against.
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.
Closes #61.
package.json'sversionis what electron-builder stamps into every artifactname and into the updater feed, and what
app.getVersion()reports — so it iswhat the About panel shows. The git tag has to agree, and nothing in CI noticed
when it didn't:
package.jsonsat at 1.2.2 while the newest release was v1.3.x,and only a human reading both numbers could tell.
What the current state actually is
Worth stating up front, because the issue's Context describes a drift that no
longer exists and an About panel that no longer hardcodes anything:
package.json1.3.0v1.3.0v1.3.0(Latest)AboutSettingswindow.api.getAppVersion()→app.getVersion(); no hardcoded string anywherescripts/check-version.mjsverify.yml/release.ymlSo the genuine gap was the guard plus its wiring, not a wrong number. Two
acceptance items are therefore already satisfied and one needed a decision — see
the table at the bottom.
The two modes, and why they differ
This is the one design decision worth reviewing, because the acceptance criterion
("
verify.ymlfails when the tag andpackage.jsondisagree") cannot beimplemented literally: on a pull request there is no tag, so that condition
only exists at release time. Splitting it is not a workaround, it is the correct
reading:
release.yml,GITHUB_REF=refs/tags/…) — the tag must equalpackage.jsonexactly. This is the release blocker.verify.yml, a PR) —package.jsonmust not be behindthe newest tag.
Compare mode deliberately does not require equality.
npm versionis mergedbefore the tag is pushed, so during that window
mainis legitimately ahead ofthe newest tag; requiring equality would fail every unrelated PR opened in
between, which would make the gate something people learn to ignore.
Failure modes it now catches, with their messages
The missing-
vcase gets its own message rather than a bare inequality, becauseit is fatal rather than cosmetic:
release.ymltriggers ontags: v*.*.*, sosuch a tag pushes nothing at all. A v-less version tag has never been pushed here,
but the release titles of
v1.1.0andv1.0.7are inconsistent, which is howclose that class of mistake has come.
Wiring
release.ymlgains aguardjob, andcreate-draftgainsneeds: guard. Amismatched tag therefore fails before the draft is created — no platform
build, and no orphan draft release left behind for a tag that was wrong. That
matters here: the file already carries a long comment about the v1.2.2 incident
where two half-populated drafts were created for one tag.
verify.ymlruns it beforenpm ci(it reads onlypackage.jsonand thetags, so a drifted version fails in seconds) and now uses
fetch-depth: 0.See the section below for why
fetch-tags: true, which was the obviousspelling, does not work.
package.jsongainscheck:version;CONTRIBUTING.mddocuments the flow.Caught by CI on the first run: the guard was silently doing nothing
Worth its own section, because it is the most useful thing this PR found and the
first version of it shipped the bug it was written to prevent.
verify.ymlwas written withfetch-tags: true, which does not do what it soundslike. The CI log from the first run:
…exit code 0. The step went green while comparing nothing, which is the one
failure mode a guard must not have — a gate that reports "no violations" without
running is worse than no gate, because everything downstream trusts it.
actions/checkoutonly fetches every tag when it fetches all history. In theshallow path it builds the refspec with
fetchTags, and that adds just thechecked-out ref's own tag — a pull request ref (
refs/pull/N/merge) has no tagat all, so nothing was fetched.
fetch-depth: 0is the fix, and at 184 commitsthe full fetch is not worth optimising.
Two changes came out of it:
fetch-depth: 0inverify.yml, with a comment recording whyfetch-tagsisnot the answer, so nobody "optimises" it back.
tolerated (running the script in a plain directory should not fail), but under
GITHUB_ACTIONSit is now a failure that names the fix. That is the samecall as
allowMissingDependencies: falseinelectron-builder.yml— a checkthat cannot run must not look like one that ran. A regression test runs the
script in a directory with no git repository to pin it.
The same run also caught a Windows-only test failure of mine:
spawnSync('npm', ...)returnsstatus: nullthere, because npm is annpm.cmdshim thatspawncannot execute withoutshell: true. The entry pointis now asserted by reading
package.jsoninstead, which is platform-independent,and CI runs the real
npm run check:versionstep anyway. (The repo's owndesignGuard.test.tsinvokesnodedirectly rather thannpm— that turns out tobe the reason.)
Review fix: pre-release precedence was not SemVer
Found in review, and a real correctness bug rather than a style nit.
compareVersionscompared pre-release strings with<, so it reportedbecause
'beta.10' < 'beta.2'is true lexicographically. That feeds straight intonewestVersionTag, so the guard could select the wrong tag as the newest releaseand then validate
package.jsonagainst it. This project has shippedv1.0.5-beta.1, so the shape is real, and the original tests only coveredalpha < betaand pre-release < final — which is exactly why they missed it.comparePrereleasenow implements SemVer 11.4 (identifiers left to right, numericcompared numerically, numeric ranks below alphanumeric, shorter set loses on a tie)
with no new dependency. The six cases that motivated it are pinned as tests, and
the end-to-end behaviour is verified in a throwaway git repository:
v1.4.0-beta.2is what the old comparison would have selected.Bare version tags were tightened in the same commit, since they produced a
confusing state.
newestVersionTagused to accept1.2.0as a release tag eventhough CONTRIBUTING requires
v<version>andrelease.ymlonly listens forv*.*.*. So a pushed2.0.0published nothing, andverify.ymlstill countedit as the newest release — which made every pull request fail with
"package.json is behind 2.0.0" over a version that never shipped. Only
v-prefixed tags count as release tags now, and a bare version tag is reportedrather than silently ignored:
This is slightly stricter than the minimum asked for (excluding bare tags would
have been enough): the tag is failed on rather than ignored, because a tag that
silently produces no release is the same class of drift this guard exists to
surface. Easy to soften if you would rather it only exclude.
Finally, "the newest release" is now "the newest version tag" — the check reads
git tag --list, not GitHub Releases.Acceptance
test/versionGuard.test.tsasserts the real exit code viaspawnSync, and the messages are shown aboveverify.ymlfails on disagreement;release.ymlrefuses to buildrelease.ymlrefuses on any tag/package.jsondisagreement;verify.ymlfails whenpackage.jsonis behind the newest tag (no tag exists on a PR)CONTRIBUTING.mddocuments the versioning flowAboutSettingsreadsapp.getVersion()over IPC. Verified by reading the call chain, not by a testOn the last row: I deliberately did not add a smoke-test assertion for it. The
smoke test could assert that the packaged
app.getVersion()equals the repo'spackage.json, but both come from the same file via electron-builder, so it wouldtest the packager rather than the acceptance criterion — it would not verify that
the About panel displays it. Adding it would have made the PR look more verified
without making it more verified. Flagging that as a judgement call.
Scope notes on things I deliberately did not do
uses:in this repo uses amutable tag, and my new job follows that. Tightening it is a repo-wide security
change touching all three workflows and does not belong in a versioning PR, but
it is a real finding if you want it as its own issue.
docs/eval. Unrelated; covered separately.CONTRIBUTING.md's command table got re-aligned by prettier because the new rowis wider. Unavoidable in a prettier-managed aligned table, and the file was
prettier-clean before, so no unrelated content moved.
Verification
npm run check:versionin every mode, including a real throwaway git repository with a bare2.0.0tag, including the behind-the-newest-tagpath induced with a temporary local tag (removed afterwards) and a no-git
directory for the "nothing to compare" branch
npm test— 254 pass / 0 fail (17 new), and again withGITHUB_ACTIONS=trueset, which is the condition that broke it earliernpm run typecheck,npm run build,npm run check:designnpm run lint— 0 errors, the pre-existing warning baseline unchangednpx prettier --checkon every touched fileguard → create-draft → release → publishNot verified
release.yml'sguardjob only runs on a tag push, so it is not exercisedhere:
verify.ymlis proven on this PR, including that the version step reallycompares rather than skipping, but the
needs: guardwiring and the tag-modebranch are only proven the first time a tag is pushed. The tag-mode logic itself
is covered by unit tests, and
release.ymlruns the same script as the PR path.Confirmed on the final run (
c2c21b8, all three platforms green):which is the line that read "no version tag reachable, nothing to compare" before
fetch-depth: 0, so the fix is verified rather than assumed.