fix(ci): accept non-closing issue references in PR linking, and add the #36850 spec - #37193
Conversation
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>
…de-24-runtime-majors
There was a problem hiding this comment.
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.mddescribing 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-artifactv8 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.
|
Claude finished @nicobytes's task in 3m 37s —— View job Code Review — PR #37193
I focused on the executable change (the workflow + its test), since New Issues
Notes (non-blocking)
Everything else in the diff checks out. The two findings are Medium (non-blocking) — worth a look before this becomes a required check, but neither blocks merge. · branch |
- 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>
|
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.
|
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>
…de-24-runtime-majors
…36850) The issue-linking gate only recognised closing keywords, so a PR that legitimately advances an issue without resolving it had no way to pass. Worse, when the link came from the branch name instead of the body, the workflow PATCHed "This PR fixes: #N" into the description — turning a non-closing situation into a closing one behind the author's back. A Spec-Kit PR 1 therefore had to either retire its parent issue on merge or leave the check red. - Accept `refs`, `references`, `related to`, `part of`, `contributes to` (same-repo, org/repo#N and full-URL forms) as valid linkage, carried through as is_closing_link=false. Checked only after every closing form fails, so an explicit `Fixes #N` still wins. - Skip the body PATCH when the link is non-closing. The PR-list comment on the issue thread still runs, so the link is tracked either way. - Match worktree-style branches (`nicobytes/36850-foo`), the dominant layout on this repo, which the old `^([0-9]+)-` anchor missed entirely. Digits must follow the slash directly, so `gh-readonly-queue/main/pr-N-<sha>` still does not match. - Rebuild the failure comment from an array: the printf format carried 21 `%s` for 22 arguments, so bash restarted the format and the last line rendered out of place. Tests extract the two parsing steps from the workflow itself and run them with the GitHub API stubbed, so they exercise the real regexes rather than copies. 29 cases, including the closing-keyword paths that must not change. Not CI-wired — .github/** has no build route today; that lands with the workflow lint job in PR 2. Refs #36850 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The timeline endpoint defaults to 30 events per page and the lookup was unpaginated, so on any PR with review history the "connected" event fell off the end. A sidebar-linked PR then read as unlinked — harmless while the only question was "linked or not", but wrong now that is_closing_link exists: the workflow reported an issue as surviving the merge while GitHub was going to close it. Found on #37193, where the connected event is the 33rd of 37: the check passed via the body reference and reported closing=false, while the sidebar link means #36850 does close on merge. Switches to `gh api --paginate` and keeps the failure path honest — the previous `$(gh ... | head -1) || fallback` form reads head's exit status, so a failing gh would never have reached the fallback. Refs #36850 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The timeline endpoint defaults to 30 events per page and the lookup was unpaginated, so on any PR with review history the "connected" event fell off the end. A sidebar-linked PR then read as unlinked — harmless while the only question was "linked or not", but wrong now that is_closing_link exists: the workflow reported an issue as surviving the merge while GitHub was going to close it. Found on #37193, where the connected event is the 33rd of 37: the check passed via the body reference and reported closing=false, while the sidebar link means #36850 does close on merge. Switches to `gh api --paginate` and keeps the failure path honest — the previous `$(gh ... | head -1) || fallback` form reads head's exit status, so a failing gh would never have reached the fallback. Refs #36850 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#36850) Review feedback: too much code for "accept Refs #N". Two cuts, no behaviour change for anything anyone asked for. - Drop the cross-repo and full-URL variants of the non-closing reference. They were symmetry with the closing paths, not a requirement — nobody has needed a non-closing link to a private-repo issue, and each form is another branch in a merge gate. Same-repo `#N` only, pinned by tests so the limitation is explicit. - Drop the six explicit `is_closing_link=true` writes. determine_issue already reads the flag as `${IS_CLOSING_LINK:-true}`, so every closing path was already covered; only the non-closing path needs to say anything. Net: the feature is 20 lines instead of 66. Refs #36850 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback, second pass: still too much code. 115 added lines -> 51. - Revert the failure comment to its printf form. Adding the two new bullets is now two argument lines plus a corrected format string, instead of a 47-line rewrite. (The format still needed correcting: it carried 21 "%s" for 22 arguments, so bash restarted the format and the last line rendered out of place. Now 24 for 24, verified by rendering.) - Drop the if/else I added around the timeline lookup. The original had no error branch there and a failed lookup already degrades to "no sidebar link", which is the same outcome the branch produced. - Tighten the comments on every remaining hunk. No behaviour change from the previous commit; the 30 tests are untouched and green. Refs #36850 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…de-24-runtime-majors
nollymar
left a comment
There was a problem hiding this comment.
Approving link-issue-to-pr changes only
…de-24-runtime-majors Resolves the conflict in .github/workflows/issue_comp_link-issue-to-pr.yml. Both sides edited the "Issue Linking Required" comment body, and both changes are wanted, so this is a semantic merge rather than picking a side: - main (6bde00d) replaced the fragile `printf "%s\n\n%s..."` format string with a quoted heredoc, because the format string kept drifting out of alignment with its arguments as lines were added, breaking list and heading rendering. - this branch had added two bullets to the printf form: the `Refs #123` / `Part of #123` option (link without closing on merge) and the `user/123-feature-description` worktree branch pattern. Kept main's heredoc structure and ported both bullets into it. Verified: link-issue-to-pr.test.sh passes 30/30. Note the suite needs jq -- without it every body-parsing case fails silently and only the branch-name cases pass, which looks like a regression and is not one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
19feb7b to
606375c
Compare
The guard script and its cicd_pr_actions-lint.yml job are removed from this change and deferred to #37194. The upgrade does not need them, and a version bump and a new permanent CI job are two changes with two different reviewers: one judging versions, the other judging whether the team wants more CI to own. AC-007 is rewritten rather than deleted, because the honest consequence has to be on the record: this change now ships NO executable test. That is a wider Principle V exception than the one first recorded, and it costs three things -- no confirmed-failing (Red) state demonstrable in CI; the ~38 files that .github/filters.yaml routes to no build keep getting no validation, which is the root cause of this issue; and nothing prevents recurrence until #37194 lands. Also corrected while here: the workflow_dispatch dry-run list named cicd_scheduled_qa-stuck-check.yml and cicd_pr_qa-stuck-check-validate.yml, both of which #37173 deleted from main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What this is
Spec-Kit PR 1 of 2 — the spec, plus one approved fix to the check that was blocking it.
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 —
Refs #36850, a deliberately non-closing link. The parent is answered by PR 2— the actual sweep of 124 action references — not by this spec, so a closing keyword here would retire
the parent the moment PR 1 merges with every stale pin still in place. The Issue Linking gate had no
way to express that, which is why this check was red. Fixing that gate is now part of this PR (see
"The second change" below), so it is green on a non-closing reference.
Follow-up scope is tracked in #37194.
Specification phase for #36850 (see the note above on why the link is non-closing). Sequencing gate is clear — #36838 landed on 2026-08-04.
The second change:
issue_comp_link-issue-to-pr.ymlSeparately approved, and the reason this PR is no longer spec-only. The linking gate recognised
only closing keywords —
fixes/closes/resolves— so a PR that advances an issue a laterPR resolves had no way to pass it. Worse, when the link came from the branch name instead of the body,
the workflow
PATCHedThis PR fixes: #Ninto the description, converting a non-closing situation intoa closing one behind the author's back. The only two outcomes were "retire the parent early" or "leave
the check red".
Three fixes:
refs,references,related to,part of,contributes to— in the same-repo,org/repo#Nand full-URL forms — satisfy the gate and setis_closing_link=false. Evaluated only after every closing form fails, so an explicitFixes #Nstill outranks a
Refs #Nelsewhere in the body.runs, so the link is tracked either way — the workflow just stops rewriting author intent.
nicobytes/36850-…/oidacra/37132-…are the dominant layouton this repo and the old
^([0-9]+)-anchor missed every one. Digits must follow the slash directly,so
gh-readonly-queue/main/pr-N-<sha>still does not match.Plus one drive-by: the failure comment's
printfcarried 21%sfor 22 arguments, so bash restartedthe format and the last line rendered out of place. It is now built from an array.
Tests —
.github/workflows/tests/link-issue-to-pr.test.sh, 29 cases. It extracts the two parsingsteps from the workflow itself and runs them with the GitHub API stubbed, so it exercises the real
regexes rather than copies of them. Included are the closing-keyword paths that must not change.
Against the pre-change workflow, 18 of the 29 fail. Not CI-wired:
.github/filters.yamlroutes nothingfrom
.github/workflows/**to a build, and this workflow has no checkout step — that gap is exactlywhat PR 2's lint job closes.
What to review
Two things, in this order:
for planning to start.
The problem
GitHub retired the Node 20 action runtime. 124 references to 8 actions across 51 files under
.github/are still onnode20-era majors (some as old ascheckout@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_VERSIONis 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: falsewhere pnpm caching is manual. That precondition doesn't exist here:setup-nodereads only$GITHUB_WORKSPACE/package.json, and this repo has no rootpackage.json(only a straypackage-lock.json).core-web/package.json'spackageManager: pnpm@10.17.1is 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-prandtest. Addinggithub-script→v8,dorny/paths-filter→v4.0.3,dawidd6/action-download-artifact→v24 anddocker/login-action→v4.6.0 closes the PR pipeline completely.github-scriptdeliberately stops at v8, a pure node24 bump with no behavior change. (An earlier draft justified this by claiming v9 breaksrequire()— that was wrong, and is corrected in the spec: v9 still injectsrequireviawrapRequire. 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'sbackendfilter only matchescicd_comp_*.yml,cicd_1-pr.ymlandcore-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. Readingsrc/unsafe-pr-checkout-helper.tsandsrc/input-helper.tsat tagv7.0.1: the guard isn't even reached for a default self-checkout.cicd_post-workflow-reporting.yml:58is a bare- uses: actions/checkout@v4with nowith:block;cicd_publish-pr-test-image.ymlhas no checkout step at all and is gated to non-forks. Conclusion: no v6 pins, and noallow-unsafe-pr-checkout: true— adding it would permanently disarm a real protection against a risk we don't have.download-artifact@v8'sdigest-mismatch: errordefault is the one genuine green-to-red risk, concentrated on the multi-GBmaven-repoartifact and onfinalize'sbuild-reports-*pattern (N artifacts = N chances to fail, on every PR and merge-queue run). PR 2 lands v8 with explicitdigest-mismatch: warnfirst — making the bump provably behavior-neutral — then flips toerrorin 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
runs.using: 'node16'(issue-fetcher,issue-labeler,changelog-report,rc-changelog) — these needdist/rebuilds, and a regeneratednccbundle can't be diff-reviewed the way YAML can.node16is more deprecated thannode20, so this should be filed immediately.aws-actions/configure-aws-credentialsv1→v6 needs OIDC and org-level trust-policy work, andslackapi/slack-github-actionv1→v2+ changes the payload format..github/dependabot.ymlfor thegithub-actionsecosystem, and extending.github/filters.yamlso.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