Repository navigation
Force node to v24 and bug fixes in the pipelines - #879
Conversation
Co-authored-by: Copilot
There was a problem hiding this comment.
Pull request overview
This PR updates GitHub Actions workflows to force JavaScript-based actions to run on Node.js 24, and includes a couple of workflow reliability fixes around tagging and release publishing.
Changes:
- Add
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: trueat workflow level across key pipelines. - Update
actions/checkoutusage inpublish.ymlfrom@v3to@v4. - Improve pipeline robustness: fix
$GITHUB_OUTPUTusage inhandle-tag.ymland avoid failing/duplicating when a GitHub Release already exists inpublish.yml.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| .github/workflows/publish.yml | Forces Node 24 for JS actions, upgrades checkout to v4, and makes release creation idempotent by skipping if the release exists. |
| .github/workflows/handle-tag.yml | Forces Node 24 for JS actions, fixes step output writing to $GITHUB_OUTPUT, and ensures version extraction waits for tag/main verification. |
| .github/workflows/claude.yml | Forces Node 24 for JS actions for the Claude workflow. |
| .github/workflows/build-test.yml | Forces Node 24 for JS actions for the build/test reusable workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude, please review this pr |
|
Claude finished @ErikMogensen's task in 1m 27s —— View job PR Review: Force node to v24 and bug fixes in the pipelines
Overall, this is a solid set of fixes. The
|
| Change | Assessment |
|---|---|
Fix broken $GITHUB_OUTPUT format |
✅ Critical correctness fix |
Add needs: [check-tag-branch] to get-version |
✅ Important pipeline gate fix |
| Idempotent release creation | ✅ Robustness improvement |
checkout@v3 → @v4 |
✅ Dependency upgrade |
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true |
✅ Correct deprecation fix |
Recommendation: Approve and merge. No blocking issues found.
paolosalvatori
left a comment
There was a problem hiding this comment.
Review summary
Overall assessment: Small, correct, and a genuine reliability improvement to the release pipeline. The important change is the $GITHUB_OUTPUT fix in handle-tag.yml: the old {name}=key::value lines defined an output literally named {name}, so tag-hash/main-hash were always empty, the verify step's if: never fired, and the "tag must be on main" gate silently passed everything. This PR makes that gate live for the first time — see the inline note for two semantics to be aware of before the next release. needs: [check-tag-branch] correctly serializes the gate ahead of get-version (and transitively the build/publish jobs). The idempotent release creation is the right replacement for continue-on-error: true, which used to swallow all errors including auth/API failures.
Key risks:
- The newly-activated tag gate compares the tag SHA to the exact current
mainHEAD and may behave surprisingly with annotated tags (inline comment onhandle-tag.yml). - Re-running the publish workflow now gets past release creation but can still fail at
gh release uploadon duplicate assets (inline comment;--clobbercloses the gap).
Status of prior review conversations:
- No inline review threads exist on this PR.
- Copilot review (2026-04-28): reviewed all 4 files, generated no comments.
- claude[bot] issue-comment review (2026-04-29) recommended approve and raised two minor points that were never replied to or addressed in code: (1)
$releaseExistsassigned but unused, (2) duplicate-asset failure on re-run of the upload step. Both still valid — re-raised as inline comments here so they can be tracked to closure. - Housekeeping: the
@claude review oncerun requested on 2026-07-17 errored out without posting (actions run 29563775453).
Recommendation: Approve / merge, ideally after adding --clobber to the upload step and giving the annotated-tag caveat a quick thought. Nothing blocking.
Co-authored-by: Paolo Salvatori
paolosalvatori
left a comment
There was a problem hiding this comment.
Thanks a lot for this one and for catching the $GITHUB_OUTPUT problem in particular. That check had been silently broken for who knows how long, so this is a genuinely valuable fix, not just housekeeping.
I approve this PR, but please take a look at the comments I left this morning and make sure to implement them. To be explicit about what I'm asking:
-
Add
--clobberto thegh release uploadstep inpublish.yml. Your existence check now lets a failed run be re-run past the create step, but the re-run will still die on assets that were already uploaded.--clobbercloses that last gap and makes the whole workflow safely re-runnable:gh release upload ${{ env.RELEASE_VERSION }} $env:ZipFilename $env:NupkgFilename --clobber
-
Drop the unused
$releaseExistsvariable — only$LASTEXITCODEis actually read. Something likegh release view ${{ env.RELEASE_VERSION }} *> $nullsays what's intended. -
Before we cut the next release, let's double-check the tag gate together. Your fix makes the "tag must be on main" check live for the first time, and it has two sharp edges: it requires the tag to match the exact current HEAD of main (tagging an older commit will now fail), and annotated tags may always fail because
github.shacan resolve to the tag object rather than the commit. If we only ever use lightweight tags we're fine, but I'd rather we verify than find out during a release. If you prefer "tag is contained in main" semantics, thegit merge-base --is-ancestorvariant I put in the review comment handles both issues.
Nothing blocking on the Node 24 changes — that's all fine, and we can delete the FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 blocks once it becomes the runner default.
Thanks again for keeping the pipelines healthy!
Paolo
|
Thanks @paolosalvatori, I updated the branch hopefully fixing 2 and 3. The issue
I think it is better to not allow overwriting a release. In case you want to overwrite a release you can delete it. |
Forced the actions to run to node v24.
Some bug fixes in the pipelines