Skip to content

docs(ci): spec for GitHub Actions Node 24 runtime upgrade (#36850) - #37193

Open
nicobytes wants to merge 6 commits into
mainfrom
nicobytes/36850-upgrade-github-actions-to-node-24-runtime-majors
Open

docs(ci): spec for GitHub Actions Node 24 runtime upgrade (#36850)#37193
nicobytes wants to merge 6 commits into
mainfrom
nicobytes/36850-upgrade-github-actions-to-node-24-runtime-majors

Conversation

@nicobytes

@nicobytes nicobytes commented Aug 24, 2026

Copy link
Copy Markdown
Member

What this is

Spec-Kit PR 1 of 2 — this PR contains spec.md and nothing else.

Per SPEC_KIT_QUICK_START.md §3, the gate is approval, not merge: once a reviewer approves this spec, planning starts. Don't wait on the merge queue. PR 2 will carry the implementation and link back here.

Issue linking — intentionally a plain reference. Parent issue: #36850. This PR carries no
auto-linking keyword, by design. The parent is answered by PR 2 — the actual sweep of 124 action
references — not by this spec, so a keyword here would retire the parent the moment PR 1 merges with
every stale pin still in place. The Issue Linking check therefore stays red on this PR: expected,
not an oversight.

Follow-up scope is tracked in #37194.

Specification phase for #36850 (see the note above on why no closing keyword is used). Sequencing gate is clear — #36838 landed on 2026-08-04.

What to review

Not the code (there is none). Review whether this is the right problem, scoped right, with measurable criteria.

The problem

GitHub retired the Node 20 action runtime. 124 references to 8 actions across 51 files under .github/ are still on node20-era majors (some as old as checkout@v2, setup-node@v2-beta). Today that means deprecation annotations on every run; when the runner drops the compatibility shim, it means the build, test and release pipeline stops working. ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION is explicitly excluded.

Three decisions in the spec worth your attention

1. AC-005 deviates from an AC written in the issue. The issue asks for package-manager-cache: false where pnpm caching is manual. That precondition doesn't exist here: setup-node reads only $GITHUB_WORKSPACE/package.json, and this repo has no root package.json (only a stray package-lock.json). core-web/package.json's packageManager: pnpm@10.17.1 is invisible to it, and v6+ limits auto-caching to npm anyway. Auto-caching cannot fire at any of the 8 sites. The spec documents the deviation rather than adding 8 lines of verifiably dead config that would get cargo-culted. This is the deviation that needs your sign-off.

2. Scope grew by 4 actions beyond the issue's list. The runner emits the annotation per job, listing that job's stale actions. Bumping only the issue's 6 clears Initial Artifact Build — satisfying the issue's literal AC — but leaves annotations on initialize, label-pr and test. Adding github-script→v8, dorny/paths-filter→v4.0.3, dawidd6/action-download-artifact→v24 and docker/login-action→v4.6.0 closes the PR pipeline completely. github-script deliberately stops at v8, a pure node24 bump with no behavior change. (An earlier draft justified this by claiming v9 breaks require() — that was wrong, and is corrected in the spec: v9 still injects require via wrapRequire. v9's real cost is Octokit v5 → v7 under 14 inline scripts, which is a different review from a runtime bump.)

3. The guard script + lint job ship in PR 2 as a droppable batch-0 commit. .github/filters.yaml's backend filter only matches cicd_comp_*.yml, cicd_1-pr.yml and core-cicd/**/action.yml — so roughly 38 of the 51 files PR 2 touches get no build, no test and no lint today. That gap is why this issue exists. Constitution Principle V also needs a committed, confirmed-failing test, and there is no unit/integration/Postman layer for workflow YAML. If you disagree, it's one self-contained commit to drop.

Risk assessment already done

Two items looked dangerous and were run to ground against upstream source, so PR 2 doesn't have to re-litigate them:

  • checkout@v7's fork-PR block cannot fire in this repo. Reading src/unsafe-pr-checkout-helper.ts and src/input-helper.ts at tag v7.0.1: the guard isn't even reached for a default self-checkout. cicd_post-workflow-reporting.yml:58 is a bare - uses: actions/checkout@v4 with no with: block; cicd_publish-pr-test-image.yml has no checkout step at all and is gated to non-forks. Conclusion: no v6 pins, and no allow-unsafe-pr-checkout: true — adding it would permanently disarm a real protection against a risk we don't have.
  • download-artifact@v8's digest-mismatch: error default is the one genuine green-to-red risk, concentrated on the multi-GB maven-repo artifact and on finalize's build-reports-* pattern (N artifacts = N chances to fail, on every PR and merge-queue run). PR 2 lands v8 with explicit digest-mismatch: warn first — making the bump provably behavior-neutral — then flips to error in a separate revertable commit.

No product surface is touched: no Java, no Angular, no DB, no ES mapping, no API contract. Nothing falls under ROLLBACK_UNSAFE_CATEGORIES.md.

Deliberately deferred to follow-ups

  • 4 first-party actions still on runs.using: 'node16' (issue-fetcher, issue-labeler, changelog-report, rc-changelog) — these need dist/ rebuilds, and a regenerated ncc bundle can't be diff-reviewed the way YAML can. node16 is more deprecated than node20, so this should be filed immediately.
  • Cold-path third-party actions (release/nightly/scheduled). Two aren't mechanical: aws-actions/configure-aws-credentials v1→v6 needs OIDC and org-level trust-policy work, and slackapi/slack-github-action v1→v2+ changes the payload format.
  • .github/dependabot.yml for the github-actions ecosystem, and extending .github/filters.yaml so .github/** gets real validation. These two are the structural fix for this class of drift — they're what makes the next runtime bump routine.

🤖 Generated with Claude Code

Issue-resolution spec (Spec-Kit PR 1) for upgrading the 124 in-scope
GitHub Actions references under .github/ to node24 majors.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 24, 2026 23:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 is Spec-Kit PR 1/2 for issue #36850, documenting the problem, scope, risks, and acceptance criteria for upgrading GitHub Actions used under .github/ to Node 24–compatible majors to eliminate Node 20 runtime deprecation annotations and prevent future CI breakage.

Changes:

  • Adds a complete spec.md describing the CI drift problem, in-scope actions, and upgrade targets.
  • Defines acceptance criteria and a proposed guard approach (script + lint job) to prevent future action-version drift.
  • Documents key risk areas (notably download-artifact v8 behavior) and explicitly deferred follow-ups.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread specs/36850-upgrade-github-actions-node-24/spec.md Outdated
Comment thread specs/36850-upgrade-github-actions-node-24/spec.md
Comment thread specs/36850-upgrade-github-actions-node-24/spec.md Outdated
@claude

claude Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nicobytes's task in 1m 2s —— View job


Review — spec-only PR

  • Read spec.md and gather context
  • Classify request (PR review; spec document, no code)
  • Check internal consistency of counts/claims
  • Recheck the three prior Copilot findings
  • Post review

This PR adds a single specification document (specs/36850-upgrade-github-actions-node-24/spec.md) and no code — so the usual bug/security/convention checks don't apply. I reviewed it as a spec: internal consistency, and whether the prior review threads are actually resolved.

Consistency checks (all pass)

  • 124 total — table sums exactly: 62 + 14 + 13 + 11 + 8 + 8 + 6 + 2 = 124. ✅
  • 8 SHA-pinned sites — checkout ×5 + download-artifact ×1 + setup-node ×1 + cache ×1 = 8, matching "Eight sites are SHA-pinned." ✅
  • README references — Problem Statement says "8 mentions across 7 README.md files (7 checkout, 1 setup-node)"; Fix Scope enumerates 5 checkout examples + 2 checkout prose + 1 setup-node prose = 7 checkout + 1 setup-node across 7 files. ✅

Resolved (prior review threads)

  • spec.md AC-005 — reworded to two grep-auditable conditions (no cache:/cache-dependency-path set and no root package.json), addressing Copilot's "tie it to the real setup-node knobs" comment. The Regression Risk section (lines 270–284) now carries the matching two-path rationale.
  • spec.md AC-007 / Red→Green — the baseline count is now explicitly informational, not part of the criterion (lines 323–327, 338–340), addressing "124 will drift on main." The AC is now "non-zero on main, zero after the sweep."
  • spec.mdgithub-script v9 rationale corrected: the false "v9 breaks require()" claim is retracted and replaced with the real cost (Octokit v5→v7 under 14 scripts), consistent with the author's comment thread.

Observations (non-blocking, author's call)

  • 🟡 The ## Assumptions section closes with "The PR ships as Spec-Kit PR 2; this spec.md ships alone as PR 1" (line 393). Read literally that's inverted — this PR (37193) is PR 1, carrying the spec alone. The intent is clear from context (it's describing how the two PRs relate), but the sentence as written reads as if the spec ships in PR 2. Worth a one-word tightening so a future reader isn't tripped by it.
  • The feature.json-tracked-on-main issue you flagged in the comment thread is real and worth the git rm --cached + gitignore, but it's correctly out of scope for this spec-only PR.

No issues found in the reviewable content — the spec is internally consistent and the three prior findings are genuinely resolved (not just acknowledged). This is a scoping/measurability review, which is exactly what a Spec-Kit PR 1 should get; the risk analysis (download-artifact digest-mismatch, checkout v7 fork-block, pnpm cache key) is unusually thorough for a runtime bump.
· nicobytes/36850-upgrade-github-actions-to-node-24-runtime-majors

nicobytes and others added 2 commits August 24, 2026 19:52
- github-script v9: correct the deferral rationale. v9 does NOT break
  require() — src/main.ts still injects wrapRequire (a Proxy over
  __non_webpack_require__ handling bare and ./-relative IDs), verified
  against tag v9.0.0. The real exposure is Octokit v5->v7 under 14
  inline scripts; require('@actions/github') and const getOctokit have
  0 occurrences here.
- AC-005: make the criterion auditable against both of setup-node's
  caching paths — no cache:/cache-dependency-path at any call site
  (explicit), and no root package.json (automatic, added in v5).
- AC-007: drop the hard-coded 124/51 baseline from the criterion; the
  count drifts with main, so the AC is now non-zero -> zero.
- Define the headline count (executable uses: lines in yml/yaml) and
  correct the README item: 8 stale refs across 7 files, including three
  prose mentions, not 5 examples.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merging main brought in #37150, which deleted core-web/libs/dotcms and
stripped `npm run build:docs:dotcms` from publish_docs.yml itself. Only
build:docs:dotcms-models survives, so the cleanup follow-up should lean
towards deletion rather than revival.

Re-verified post-merge: setup-node@v2-beta + node-version 16.13.2 are
still present there, so the in-scope pin bump is unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nicobytes

Copy link
Copy Markdown
Member Author

Thanks — all three review points are addressed in 1d216ff and c3a5c23. On the Medium, you caught a real error in my rationale, but the proposed replacement isn't right either, so I went to the source.

github-script v9 and require()

You're correct that the three cited sites don't require('@actions/github') — they do require('fs') (×2) and require('./.github/scripts/qa-stuck-check/find-stuck-issues.js'). My rationale was wrong.

But the suggested stronger reading — that v9's ESM scope makes any require unavailable — doesn't hold. Verified against tag v9.0.0:

  • src/main.ts still injects it: require: wrapRequire and __original_require__: __non_webpack_require__.
  • src/wrap-require.ts is a Proxy over __non_webpack_require__ that handles both forms — moduleID.startsWith('.')path.resolve then delegate; otherwise target.resolve(moduleID, {paths: [process.cwd()]}).
  • src/async-function.ts still declares require: NodeRequire and __original_require__: NodeRequire in AsyncFunctionArguments.

So require('fs') and the local-module require would both keep working on v9. What v9 actually breaks, per its release notes: require('@actions/github') specifically (that package is now ESM-only) — 0 occurrences here; and getOctokit becoming an injected parameter, making const/let getOctokit a SyntaxError — also 0 occurrences here.

So why still stop at v8?

Not because v9 breaks — it probably doesn't, for this repo. Because v8 is a zero-behavior-change node24 bump, which is the entire ask of this issue, whereas v9 additionally moves Octokit v5 → v7 underneath 14 inline scripts. Auditing 14 scripts against an Octokit major is a different review from a runtime bump, and mixing it in is how a mechanical PR becomes an unreviewable one. The spec now says exactly that, and drops the false require() claim.

Net effect: same conclusion, correct reason, and the deferred v9 audit is now scoped to the right break.

Also worth flagging, unrelated to this PR

Merging main into this branch surfaced that .specify/feature.json is now tracked on main (added by #37150), containing {"feature_directory":"specs/36950-remove-dead-core-web-libs"}. Per SPEC_KIT_QUICK_START.md that file is local, untracked, and committed to neither PR — it's a per-developer pointer. Tracked, it silently redirects everyone's next Spec-Kit command to someone else's feature directory. Out of scope here (this PR is spec-only), but it should be git rm --cached'd and gitignored.

AC-001 requires residual Node deprecation annotations to be attributable
to a *named* follow-up issue; the spec described one without naming it.
Points the four deferral sites at #37194.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

❌ Issue Linking Required

This PR could not be linked to an issue. All PRs must be linked to an issue for tracking purposes.

How to fix this:

Option 1: Add keyword to PR body (Recommended - auto-removes this comment)
Edit this PR description and add one of these lines:

  • Fixes #123 or Closes #123 — same-repo issue
  • Fixes org/repo#123 or Closes org/repo#123 — cross-repo or private issue
  • Closes https://github.com/org/repo/issues/123 — full GitHub URL
  • Other supported keywords: fix, fixed, close, closed, resolve, resolved

Option 2: Link via GitHub UI (Note: won't clear the failed check)

  1. Go to the PR → Development section (right sidebar)

  2. Click "Link issue" and select an existing issue

  3. Push a new commit or re-run the workflow to clear the failed check
    Option 3: Use branch naming
    Create a new branch with one of these patterns:

  • 123-feature-description (number at start)

  • issue-123-feature-description (issue-number at start)

  • feature-issue-123 (issue-number anywhere)

Why is this required?

Issue linking ensures proper tracking, documentation, and helps maintain project history. It connects your code changes to the problem they solve.
---This comment was automatically generated by the issue linking workflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants