Skip to content

ci: test the declared Node floor on every push (#1356) - #1365

Open
prx-my wants to merge 6 commits into
OWASP:mainfrom
prx-my:ci/issue-1356-node-floor
Open

prx-my wants to merge 6 commits into
OWASP:mainfrom
prx-my:ci/issue-1356-node-floor

Conversation

@prx-my

@prx-my prx-my commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

What changed and why

engines.node claimed >=20, but every workflow pinned Node 24, so the declared floor was never exercised. This raises the declaration to >=22 (the floor #1237 already chose) and adds a CI job that actually runs the suite there.

  • package.json / package-lock.json: engines.node -> >=22
  • .github/workflows/ci.yml: new node-floor job at Node 22
  • website/docs/troubleshooting.md: minimum requirement -> node >= 22

Design choices

Separate job, not a matrix. The existing job is the required build status check. Matrixing it would rename the contexts to build (22) / build (24), so the required build context would never report and every PR would be blocked. The existing job and its name are unchanged.

The new job keeps the advisory sync. The --offline e2e tests need a synced database; skipping the sync would make them fail rather than merely reduce coverage, so the new job mirrors build end to end. It roughly doubles the sync work per push, which seemed the honest trade for a floor that is genuinely tested.

Verification

  • npm run build clean on Node 22 and Node 24
  • Full suite green on Node 22.23.3: 170 suites, 2406 tests
  • Full suite green on Node 26.8.2: 170 suites, 2406 tests
  • actionlint clean
  • Existing build job untouched, so the required check keeps reporting under its current name

The final check from the issue - breaking something version-sensitive and watching node-floor go red - is being run on this PR next; I will note the result here.

Closes #1356

Raise engines.node to >=22, which is the floor OWASP#1237 already chose, and
add a separate Node 22 job so the declared floor is exercised in CI. The
existing build job keeps its name because it is a required status check.

The new job mirrors build, including the advisory sync: the offline e2e
tests fail without a synced database, so skipping it would remove
coverage rather than just save time.
@prx-my
prx-my requested a review from sonukapoor as a code owner October 21, 2026 05:42
Copilot AI balanced review requested due to automatic review settings October 21, 2026 05:42
@cve-lite-cli-bot

cve-lite-cli-bot Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

CVE Lite CLI

No new vulnerabilities introduced by this PR

Posted because this PR changes dependencies. Nothing is posted when they are untouched.

Scanned by CVE Lite CLI, a free and open source OWASP project.

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@prx-my

prx-my commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Probe result, as the issue asked:

This also confirms the required build check still reports under its existing name, so existing PRs are not blocked.

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.

🟢 Approval recommended

No unresolved issues remain, and the review evidence confirms both CI jobs pass and the new job detects version-sensitive failures.

No open issues 🎉

Review effort: Balanced

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved, thanks @prx-my.

The part I cared most about was whether the new Node 22 job can actually fail, and your probe settles it: a test that only passes on Node 24 turns the 22 job red while the existing build stays green. The engines floor matches in both the manifest and the lockfile, the job only has read access, and the docs agree with it.

I merge these one at a time rather than in a batch, so it will land shortly.

This branch has not been deployed

No deployments
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.

ci: we declare Node >=20 and only ever test Node 24, so the floor we claim is untested

3 participants