ci: gate the publish job, keep --test-force-exit (verified handle leak) - #39
Merged
Conversation
Picks up part of the node-red-standards 0.4.1 fixes.
npm-publish.yml had one job that ran `npm ci`, `npm test`, `npm publish` in
sequence — no lint, no format:check, and no job separation, so nothing stood
between a tag and an irreversible publish except the test suite. Replaced with
the standard's two-job shape: `build` runs lint, format:check and test, and
`publish-npm` declares `needs: build`. A release is cut from a tag and nothing
guarantees that tag points at a commit CI ever saw.
It also published with a plain `npm publish`, so a release flagged pre-release
would have gone to `latest` and auto-upgraded every Manage Palette user onto an
unproven build; `latest` cannot be walked back by publishing, only by a separate
dist-tag change. Now routed to the `beta` dist-tag.
Node stays on 20.x rather than the template's 22.x, which exists for repos that
raised their floor above the standard's >=20 baseline; this package declares >=20.
standards-check.yml already used the correct Git-resolved invocation, so this is
only a refresh from the template: same command, plus the `contents: read`
permissions block.
The test script deliberately KEEPS --test-force-exit, against the standard.
The standard dropped the flag because it masks handle leaks, and this repo turns
out to have one: without it the suite hangs rather than exiting. Measured — full
run killed at the 480 s timeout (exit 124) after 204 passing tests with over
seven minutes of no output, and the culprit reproduces in isolation:
node --test --test-timeout=30000 --test-concurrency=1 test/login-endpoints.test.js
-> 16 pass, then hangs (exit 124)
--test-timeout does not catch it, because the leak holds the process open after
the tests themselves have finished. So this is exactly the trade-off the standard
documents: the flag is what keeps the run from hanging, and removing it has to
wait until the open handle in test/login-endpoints.test.js is closed. Keeping it
is a known, recorded deviation rather than an oversight.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
Picks up part of the node-red-standards 0.4.1 fixes. The test-script change is deliberately not taken — see below.
npm-publish.yml— the release had almost no gateIt was one job running
npm ci→npm test→npm publish. No lint, noformat:check, and no job separation, so nothing stood between a tag and an irreversible publish except the test suite. Now the standard's two-job shape:buildruns lint, format:check and test, andpublish-npmdeclaresneeds: build.It also used a plain
npm publish, so a release flagged pre-release would have gone tolatestand auto-upgraded every Manage Palette user onto an unproven build. Now routed to thebetadist-tag.Node stays on 20.x rather than the template's 22.x — that pin exists for repos that raised their floor above the standard's
>=20baseline. This package declares>=20.standards-check.yml— refresh onlyThis repo already had the correct Git-resolved invocation (it was ahead of the standard here). Same command, plus the
contents: readpermissions block.--test-force-exitThe standard dropped the flag because it masks handle leaks. This repo has one. Without the flag the suite doesn't finish — it hangs:
--test-timeoutdoes not catch it, because the leak holds the process open after the tests themselves have finished.This is precisely the trade-off the standard documents: the flag is what stops the run hanging, and it can only come out once the open handle in
test/login-endpoints.test.jsis closed. Worth fixing separately — a suite that exits on its own also proves it leaks nothing. Until then this is a recorded deviation, not an oversight.🤖 Generated with Claude Code