Skip to content

Force node to v24 and bug fixes in the pipelines - #879

Merged
ErikMogensen merged 3 commits into
paolosalvatori:mainfrom
ErikMogensen:fix-pipelines
Jul 17, 2026
Merged

ErikMogensen merged 3 commits into
paolosalvatori:mainfrom
ErikMogensen:fix-pipelines

Conversation

@ErikMogensen

Copy link
Copy Markdown
Collaborator

Forced the actions to run to node v24.
Some bug fixes in the pipelines

Copilot AI review requested due to automatic review settings April 28, 2026 16:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: true at workflow level across key pipelines.
  • Update actions/checkout usage in publish.yml from @v3 to @v4.
  • Improve pipeline robustness: fix $GITHUB_OUTPUT usage in handle-tag.yml and avoid failing/duplicating when a GitHub Release already exists in publish.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.

@ErikMogensen

Copy link
Copy Markdown
Collaborator Author

@claude, please review this pr

@claude

claude Bot commented Apr 29, 2026 •

Copy link
Copy Markdown

Claude finished @ErikMogensen's task in 1m 27s —— View job


PR Review: Force node to v24 and bug fixes in the pipelines

  • Read changed workflow files
  • Analyze changes
  • Post review feedback

Overall, this is a solid set of fixes. The $GITHUB_OUTPUT corrections are particularly important — the old format was silently broken and job outputs were never being set correctly, which would have caused downstream jobs to behave unpredictably. Everything else is a good incremental improvement.

handle-tag.yml — Critical bug fixes

$GITHUB_OUTPUT format fix (lines 21, 33)

The old format was wrong on both output steps:

# Old (broken)
echo "{name}=tag-hash::${hash}" >> $GITHUB_OUTPUT
echo "{name}=main-hash::${hash}" >> $GITHUB_OUTPUT

# New (correct)
echo "tag-hash=${hash}" >> $GITHUB_OUTPUT
echo "main-hash=${hash}" >> $GITHUB_OUTPUT

The {name}=key::value pattern was a leftover from the deprecated ::set-output command era. The correct modern format is key=value. With the broken format, steps.tag-commit-hash.outputs.tag-hash and steps.main-commit-hash.outputs.main-hash would always be empty strings, making the branch-verification check on line 37 always evaluate to false (i.e., always pass, even for tags not on main). This was a real security/correctness gap — good catch.

needs: [check-tag-branch] added to get-version (line 43)

Before this change, get-version ran concurrently with check-tag-branch. If the tag was not on main, check-tag-branch would fail, but get-version (and subsequently call-build-test and publish-it) could still be triggered in parallel since they only declared needs: [check-tag-branch, get-version] — not a strict sequential dependency on check-tag-branch completing first. Adding needs: [check-tag-branch] to get-version properly gates the entire pipeline. Correct fix.

publish.yml — Idempotent release creation

Replacing continue-on-error: true with explicit existence check (lines 86–92)

$releaseExists = gh release view ${{ env.RELEASE_VERSION }} 2>&1
if ($LASTEXITCODE -ne 0) {
  gh release create ${{ env.RELEASE_VERSION }} --draft --generate-notes
} else {
  echo "Release ${{ env.RELEASE_VERSION }} already exists, skipping creation."
}

This is better than continue-on-error: true because it only swallows the "release already exists" case rather than silently ignoring all errors (permissions issues, API failures, etc.). The logic is correct — $LASTEXITCODE is the right check here for PowerShell.

Minor style note: $releaseExists is assigned but its content is never used — only $LASTEXITCODE matters. Could optionally redirect output to $null instead (gh release view ... > $null 2>&1), but this works fine as-is.

One potential edge case: if this workflow is triggered twice for the same tag and the first run already uploaded release assets, the second run's gh release upload step (line 99–100) could fail with a duplicate-asset error, since continue-on-error was only removed from the create step. This is an uncommon scenario, but worth being aware of.

actions/checkout@v3 → @v4 (line 28)

Standard upgrade. v3 uses Node.js 16 (deprecated) while v4 uses Node.js 20. This is a necessary bump and pairs well with FORCE_JAVASCRIPT_ACTIONS_TO_NODE24.

build-test.yml, claude.yml, handle-tag.yml, publish.yml — FORCE_JAVASCRIPT_ACTIONS_TO_NODE24

Setting FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true at workflow level is the correct approach to suppress Node.js deprecation warnings from older bundled actions. All four workflows apply it consistently.

Summary

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.

Repository owner deleted a comment from claude Bot Jul 17, 2026

@paolosalvatori paolosalvatori left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 main HEAD and may behave surprisingly with annotated tags (inline comment on handle-tag.yml).
  • Re-running the publish workflow now gets past release creation but can still fail at gh release upload on duplicate assets (inline comment; --clobber closes 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) $releaseExists assigned 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 once run 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.

Comment thread .github/workflows/handle-tag.yml Outdated
Comment thread .github/workflows/publish.yml Outdated
Comment thread .github/workflows/publish.yml
Comment thread .github/workflows/build-test.yml
@paolosalvatori
paolosalvatori self-requested a review July 17, 2026 09:10

@paolosalvatori paolosalvatori left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ErikMogensen

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:

  1. Add --clobber to the gh release upload step in publish.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. --clobber closes that last gap and makes the whole workflow safely re-runnable:

    gh release upload ${{ env.RELEASE_VERSION }} $env:ZipFilename $env:NupkgFilename --clobber
  2. Drop the unused $releaseExists variable — only $LASTEXITCODE is actually read. Something like gh release view ${{ env.RELEASE_VERSION }} *> $null says what's intended.

  3. 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.sha can 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, the git merge-base --is-ancestor variant 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

@ErikMogensen

Copy link
Copy Markdown
Collaborator Author

Thanks @paolosalvatori, I updated the branch hopefully fixing 2 and 3. The issue

  1. Add --clobber to the gh release upload step in publish.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. --clobber closes that last gap and makes the whole workflow safely re-runnable:

I think it is better to not allow overwriting a release. In case you want to overwrite a release you can delete it.

@ErikMogensen
ErikMogensen merged commit 174eba9 into paolosalvatori:main Jul 17, 2026
4 checks passed
@ErikMogensen
ErikMogensen deleted the fix-pipelines branch July 17, 2026 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants