Skip to content

fix(deps): declare the Node 22 floor the repo already builds on - #1479

Merged
groupthinking merged 3 commits into
mainfrom
claude/clever-heisenberg-8ktmyz
Aug 13, 2026
Merged

fix(deps): declare the Node 22 floor the repo already builds on#1479
groupthinking merged 3 commits into
mainfrom
claude/clever-heisenberg-8ktmyz

Conversation

@groupthinking

@groupthinking groupthinking commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Canonical issue

No pre-existing issue — found during a scheduled PR-remediation sweep while auditing what the recent merge burst landed on main. Filing the fix directly rather than opening an issue to close in the same breath.

Outcome

engines.node advertised >=20.6.0 while nothing in the repo has built or tested on Node 20 for some time. Vercel reads engines.node to select the runtime for apps/web, so the stale floor let it pick a Node 20 runtime on which several core runtime dependencies are unsupported. The declaration now matches what is actually built, tested and shipped.

Evidence the floor was stale:

  • ci.yml, e2e-tests.yml and security.yml all pin node-version: 22.
  • apps/web/Dockerfile builds FROM node:22-slim.
  • The lockfile records a bare >=22 for openai@7.3.0 (>=22.0.0), ai@7.0.47 (>=22), @ai-sdk/gateway@4.0.36 (>=22) and the @supabase/* client set (>=22.0.0).

The gap was silent by construction: npm install only emits an EBADENGINE warning when a package wants a newer Node than the root declares, so no check ever went red.

Scope

  • Included:
    • package.jsonengines.node >=20.6.0>=22.0.0; engines.npm >=8.0.0>=10.0.0 (Node 22 ships npm 10, so >=8.0.0 described a combination that cannot occur).
    • tests/unit/test_node_engines_floor.py — new; ties the advertised major to the CI pins, the production image tag, and the core runtime dependencies' locked floors.
  • Explicitly excluded:
    • No dependency versions changed; no lockfile change.
    • Not asserting that the floor dominates every dependency's declared range — see the correction below for why that would be unsound, not merely strict.
    • One adjacent observation left alone as a separate concern: chrome-devtools-mcp sits in root dependencies rather than devDependencies.

Correction (second commit, 339ac79)

The first commit's rationale contained a factual error, caught by CodeRabbit's review and fixed here.

_parse_floor used re.search, so on a union range it pulled out the trailing >= branch and reported it as a minimum. That produced the claim — in the original commit message, the module docstring, and this PR body — that vitest@4.1.10 and eslint-visitor-keys@5.0.1 "declare >=24". They do not:

package actual range admits Node 22?
vitest@4.1.10 ^20.0.0 || ^22.0.0 || >=24.0.0 yes
eslint-visitor-keys@5.0.1 ^20.19.0 || ^22.13.0 || >=24 yes
chrome-devtools-mcp@1.6.0 ^20.19.0 || ^22.12.0 || >=23 yes
vite@8.2.0 ^20.19.0 || >=22.12.0 yes

The trailing >= clause in each is one alternative among several, not an unconditional requirement. Nothing in the tree conflicts with Node 22; the "dev tooling wants 24" framing was an artefact of the buggy parser.

The regex is now anchored, so a range reduces to a floor only when it is entirely one >=X[.Y[.Z]] clause. Unions and compound ranges return None, and a selected runtime dependency that adopts such a form fails loudly with a message telling the reader to re-read the range by hand rather than passing vacuously. The scope boundary itself is unchanged and, per CodeRabbit, correct.

Also in that commit: workflow Node pins are now read with yaml.safe_load and extracted from with["node-version"], matching the precedent in tests/unit/test_auto_label_workflow.py. The previous text match would have counted a commented-out # node-version: 20 as a pin and failed the test while the workflow still ran Node 22. The Dockerfile check stays a text assertion — it anchors on FROM at line start and so cannot match a comment.

Risk

  • Risk level: low
  • Failure mode: a contributor or deploy target pinned to Node 20.x now gets an EBADENGINE warning (a hard failure only under engine-strict, which this repo does not set — root .npmrc sets only legacy-peer-deps=true). That surfaces a configuration already broken for openai@7/ai@7/Supabase rather than introducing a new break. CI, Docker and Vercel all already run Node 22 and are unaffected.
  • Rollback: revert both commits; engines returns to >=20.6.0 / >=8.0.0 and the new test file goes with it.

Verification

Tied to head 339ac79.

The broader Python suite could not be run in this sandbox: pytest tests/unit/ hits 71 collection errors, all ModuleNotFoundError: No module named 'fastapi' and friends. That is a missing-dependency limitation of the environment, not a defect on main; the suites above are stdlib+PyYAML only and run clean.

Production evidence

The Vercel preview for this branch built and reached Ready under engines.node: ">=22.0.0" — a successful install and Next.js build under the new floor is the direct evidence, since the change affects runtime selection at install/deploy time rather than any request path.

Agent handoff

  • One canonical issue is linked — n/a, no issue exists; rationale above.
  • No competing PR implements the same issue — no open PR touches root engines.
  • Acceptance criteria satisfied for the engine-floor declaration.
  • Required engineering checks expected to pass on the current head.
  • Human decision requested for the merge gate below.

Agent provenance

Produced by a scheduled, unattended PR-remediation routine. Kept as draft and halted at the human/governance gate: this routine's runbook states that it must never auto-merge to protected main.

Context worth flagging on this PR specifically: while this sweep ran, ~17 PRs were merged into main between 20:43 and 20:50 UTC by a separate unattended process — several flipped from draft to ready and merged within seconds, with their check runs still queued at merge time. This PR is deliberately not following that pattern. The repo owner has been notified separately.

`engines.node` said `>=20.6.0`, but nothing in the repo has built or tested
on Node 20 for some time:

- `ci.yml`, `e2e-tests.yml` and `security.yml` all pin `node-version: 22`.
- `apps/web/Dockerfile` builds `FROM node:22-slim`.
- Core runtime dependencies declare `>=22.0.0` in the lockfile — `openai@7.3.0`,
  `ai@7.0.47`, `@ai-sdk/gateway@4.0.36` and the whole `@supabase/*` client set.

The gap was silent by construction. `npm install` only emits an `EBADENGINE`
warning when a package wants a newer Node than the root declares, so no check
ever went red. The consequence is on Vercel, which reads `engines.node` to
select the runtime for `apps/web`: a floor of `>=20.6.0` permits it to pick a
Node 20 runtime on which those dependencies are unsupported.

Raise the floor to `>=22.0.0` so the advertised support matches what is
actually built, tested and shipped. `npm` moves to `>=10.0.0` to match — Node
22 ships npm 10, so `>=8.0.0` described a combination that cannot occur.

`tests/unit/test_node_engines_floor.py` locks the invariant to the toolchain:
the advertised major must equal the major every CI workflow pins and the major
the production image ships, and must satisfy the core runtime dependencies'
locked floors. Against the previous `>=20.6.0` those five assertions fail; with
this change all ten pass.

Deliberately not asserted is that the floor dominates *every* dependency's
declared floor. Some dev tooling (`vitest@4.1.10`, `eslint-visitor-keys@5.0.1`)
declares `>=24` and some optional platform binaries declare `>=22.12`, yet CI
is green on Node 22 because those floors are advisory. Encoding dominance would
assert a rule the repo does not follow and would force the floor past the
version it actually runs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NBdBTcFjhyjyKP9bDuj4gZ
@vercel

vercel Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
v0-uvai Ready Ready Preview, v0 Aug 7, 2026 9:01pm

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@groupthinking, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 111 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c9ce5e3a-ffb4-4b0e-b893-178f8ff83506

📥 Commits

Reviewing files that changed from the base of the PR and between 12f5c44 and 38a4ea6.

⛔ Files ignored due to path filters (1)
  • tests/unit/test_node_engines_floor.py is excluded by !tests/**
📒 Files selected for processing (1)
  • package.json

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

Auto-review was skipped by the label gate (this PR carried no labels at open time). Invoking explicitly.

Focus areas for the review:

  1. tests/unit/test_node_engines_floor.py — the semver floor parser is deliberately minimal. It only understands a leading >=X[.Y[.Z]] clause, which is the only form present in this tree's engines declarations. Is that too fragile if a dependency later ships a compound range (>=22 <24) or an || union? It would currently parse the first >= and ignore the rest.
  2. The _NODE_VERSION_KEY and _DOCKER_NODE regexes read workflow YAML and the Dockerfile as text rather than parsing them. test_auto_label_workflow.py in this repo uses yaml.safe_load instead. Is the text-matching approach here a correctness risk (e.g. a commented-out node-version: line would be picked up and would fail the test spuriously)?
  3. Scope judgement: the test asserts the declared floor matches the CI/Docker pins and satisfies three named runtime deps, but deliberately does not assert it dominates every dependency's floor — vitest@4.1.10 and eslint-visitor-keys@5.0.1 declare >=24 while CI is green on Node 22, so dominance would encode a rule the repo doesn't follow. Reasonable line to draw, or should the stricter invariant be asserted with an explicit allowlist for the advisory cases?

Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- PR diff summary ---'
git diff --stat HEAD^ HEAD
printf '%s\n' '--- changed files ---'
git diff --name-status HEAD^ HEAD

printf '%s\n' '--- test outline ---'
ast-grep outline tests/unit/test_node_engines_floor.py --items all --view expanded || true

printf '%s\n' '--- test source ---'
cat -n tests/unit/test_node_engines_floor.py

printf '%s\n' '--- root package engines ---'
jq '{engines, dependencies, devDependencies}' package.json

printf '%s\n' '--- workflow node-version entries, including comments ---'
rg -n -C 3 'node-version\s*:' .github/workflows || true

printf '%s\n' '--- Docker node image entries, including comments ---'
rg -n -C 3 '(?i)\bfrom\s+node:' apps/web/Dockerfile || true

printf '%s\n' '--- candidate dependency engine metadata from lockfile ---'
jq -r '
  .packages
  | to_entries[]
  | select(.key == "node_modules/openai"
        or .key == "node_modules/ai"
        or .key == "node_modules/@ai-sdk/gateway"
        or .key == "node_modules/vitest"
        or .key == "node_modules/eslint-visitor-keys")
  | "\(.key): \(.value.engines.node // "<no engines.node>")"
' package-lock.json

printf '%s\n' '--- YAML parser precedent ---'
fd -a '^test_auto_label_workflow\.py$' . -x sh -c 'echo "--- $1"; cat -n "$1"' sh {} \;

Length of output: 2099


🏁 Script executed:

#!/bin/bash
set -u -o pipefail

printf '%s\n' '--- commit tree paths ---'
git ls-tree -r --name-only HEAD | rg -n '(^tests/unit/test_node_engines_floor\.py$|^package\.json$|^package-lock\.json$|^apps/web/Dockerfile$|^\.github/workflows/.*\.ya?ml$|test_auto_label_workflow\.py$)' || true

printf '%s\n' '--- added test from commit ---'
git show HEAD:tests/unit/test_node_engines_floor.py | nl -ba || true

printf '%s\n' '--- package engines from commit ---'
git show HEAD:package.json | jq '{engines, dependencies, devDependencies}' || true

printf '%s\n' '--- node-version text in committed workflows ---'
while IFS= read -r workflow; do
  printf '%s\n' "--- $workflow"
  git show "HEAD:$workflow" | rg -n -C 3 'node-version\s*:' || true
done < <(git ls-tree -r --name-only HEAD .github/workflows | rg '\.ya?ml$')

printf '%s\n' '--- node image text in committed Dockerfile ---'
git show HEAD:apps/web/Dockerfile | rg -n -C 3 '(?i)\bfrom\s+node:' || true

printf '%s\n' '--- selected dependency engine metadata from committed lockfile ---'
git show HEAD:package-lock.json | jq -r '
  .packages
  | to_entries[]
  | select(.key == "node_modules/openai"
        or .key == "node_modules/ai"
        or .key == "node_modules/@ai-sdk/gateway"
        or .key == "node_modules/vitest"
        or .key == "node_modules/eslint-visitor-keys")
  | "\(.key): \(.value.engines.node // "<no engines.node>")"
' || true

printf '%s\n' '--- YAML parser precedent from commit ---'
while IFS= read -r path; do
  printf '%s\n' "--- $path"
  git show "HEAD:$path" | nl -ba
done < <(git ls-tree -r --name-only HEAD | rg '(^|/)test_auto_label_workflow\.py$')

Length of output: 4887


🏁 Script executed:

#!/bin/bash
set -u -o pipefail

number_lines() {
  awk '{printf "%6d\t%s\n", NR, $0}'
}

printf '%s\n' '--- test source from commit ---'
git show HEAD:tests/unit/test_node_engines_floor.py | number_lines || true

printf '%s\n' '--- node pinning workflow source from commit ---'
for workflow in .github/workflows/ci.yml .github/workflows/e2e-tests.yml .github/workflows/security.yml; do
  printf '%s\n' "--- $workflow"
  git show "HEAD:$workflow" | number_lines || true
done

printf '%s\n' '--- Dockerfile source from commit ---'
git show HEAD:apps/web/Dockerfile | number_lines || true

printf '%s\n' '--- YAML parser precedent from commit ---'
git show HEAD:tests/unit/test_auto_label_workflow.py | number_lines || true

printf '%s\n' '--- all engine ranges that start with >= in committed lockfile ---'
git show HEAD:package-lock.json | jq -r '
  .packages | to_entries[]
  | select(.value.engines.node? != null)
  | [.key, .value.engines.node] | `@tsv`
' | rg $'\t>=|\\|\\|' | head -n 160 || true

Length of output: 45087


@groupthinking Full review complete. I found two test-correctness issues.

  1. _parse_floor() does not safely support union ranges.
    Line 46 uses search(). It finds a >= clause anywhere in the range. For example, ^20.19.0 || ^22.0.0 || >=24.0.0 accepts Node 22, but _parse_floor() returns (24, 0, 0). A future selected runtime dependency with that form would make Line 124 fail incorrectly.

    Do not treat a later >= branch as a universal minimum. Either:

    • reject every range except one complete >=X[.Y[.Z]] clause, or
    • evaluate whether the declared Node version satisfies the complete semver range.

    The first option fits the stated minimal-parser design. Add parser cases for compound and union ranges, and fail with a clear message when a selected dependency changes to an unsupported range form.

  2. _NODE_VERSION_KEY can match comments and unrelated YAML text.
    Line 47 is unanchored and Line 95 scans raw workflow text. A line such as # node-version: 20 would add 20 to pinned and fail the test. The workflow would still use Node 22.

    Load each workflow with yaml.safe_load, as tests/unit/test_auto_label_workflow.py already does. Extract jobs.*.steps[*].with["node-version"] from the parsed mapping. This removes comment and scalar false positives. The Dockerfile regex does not have the same commented-line problem because it requires FROM at the start of the line. It should remain a narrow text assertion unless the Dockerfile begins to use variable-based image references or FROM flags.

The scope boundary is correct. Keep the invariant limited to the CI pins, production image, and selected direct runtime dependencies. Do not require the root floor to dominate every lockfile engines.node range. The current vitest and eslint-visitor-keys ranges include Node 22 alternatives; their >=24 branches are not unconditional requirements.


✏️ Learnings added
Learnt from: groupthinking
URL: https://github.com/groupthinking/EventRelay/pull/1479

Timestamp: 2026-08-07T20:57:57.046Z
Learning: For EventRelay, the root `package.json` Node engine floor must match the Node major pinned in `.github/workflows/ci.yml`, `.github/workflows/e2e-tests.yml`, `.github/workflows/security.yml`, and `apps/web/Dockerfile`. Regression checks should validate selected direct runtime dependencies rather than enforce a global maximum over every package-lock `engines.node` range, because development and optional-platform dependencies can use union ranges that still support Node 22.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

⚠️ Action not completed

Review rate limited.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 52 minutes.

Addresses both findings from CodeRabbit's review on #1479.

`_parse_floor` used `re.search`, so it pulled the trailing `>=` clause out of
a union range and reported it as a floor. That is wrong, and it misled the
original analysis of this very change: `vitest@4.1.10` is
`^20.0.0 || ^22.0.0 || >=24.0.0` and `eslint-visitor-keys@5.0.1` is
`^20.19.0 || ^22.13.0 || >=24` — both explicitly admit Node 22. They were
described as "requiring >=24" only because the parser said so. The same
applies to `chrome-devtools-mcp@1.6.0` and `vite@8.2.0`.

The regex is now anchored, so a range is reduced to a floor only when it is
entirely one `>=X[.Y[.Z]]` clause. Unions and compound ranges return `None`,
and a selected runtime dependency that adopts such a form now fails with a
message telling the reader to re-read the range by hand rather than letting
the assertion pass vacuously. Parser cases cover the four real union ranges in
the lockfile plus `>=22 <24`.

Workflow Node pins are now read with `yaml.safe_load` and extracted from
`with["node-version"]`, matching the precedent in
`tests/unit/test_auto_label_workflow.py`. The previous text match would have
counted a commented-out `# node-version: 20` as a pin and failed the test while
the workflow still ran Node 22. Covered by a test using a fixture that contains
a commented pin and a prose mention. The Dockerfile check stays a text
assertion: it anchors on `FROM` at line start and so cannot match a comment.

PyYAML is already a declared dependency (`pyproject.toml`, `requirements.txt`)
and several existing workflow tests import it.

No change to the invariant or its scope: floor vs CI pins, production image,
and the three selected runtime dependencies. `pytest
tests/unit/test_node_engines_floor.py` -> 19 passed; reverting `engines` to
`>=20.6.0` still fails exactly the 5 assertions that encode the fix.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NBdBTcFjhyjyKP9bDuj4gZ

Copy link
Copy Markdown
Owner Author

Both findings were correct and are fixed in 339ac79. Finding 1 was the more serious of the two, because the bug had propagated into my own reasoning.

1. Union ranges — fixed, and it invalidated a claim in the original PR body.

You're right that search() treats a trailing >= branch as a universal minimum. I'd taken the "reject anything but a single complete >= clause" option, as you suggested it fits the minimal-parser design. _MIN_VERSION is now anchored (^...$), so unions and compound ranges return None.

The part worth calling out: this bug is why the first commit claimed vitest and eslint-visitor-keys "declare >=24". They don't — I read that off the broken parser rather than the lockfile. The actual ranges all admit Node 22:

package actual range
vitest@4.1.10 ^20.0.0 || ^22.0.0 || >=24.0.0
eslint-visitor-keys@5.0.1 ^20.19.0 || ^22.13.0 || >=24
chrome-devtools-mcp@1.6.0 ^20.19.0 || ^22.12.0 || >=23
vite@8.2.0 ^20.19.0 || >=22.12.0

So there is no "dev tooling wants Node 24 while CI runs 22" tension in this repo — that was an artefact. I've corrected the PR body and the module docstring. All four ranges above are now pinned as parser cases expecting None, alongside >=22 <24.

On the failure mode you asked for: a selected runtime dependency that moves to an unsupported form now fails with a message telling the reader to re-read the range by hand and either widen the parser or drop it from RUNTIME_DEPS_UNDER_TEST — explicitly not to let the assertion pass vacuously.

2. YAML parsing — fixed.

Workflow pins now come from yaml.safe_load and with["node-version"], following tests/unit/test_auto_label_workflow.py. Covered by a fixture containing both a commented # node-version: 18 and a prose mention, asserting only {22} is extracted. Agreed on leaving the Dockerfile as a text assertion — it anchors FROM at line start, so the commented-line failure mode doesn't apply. Worth revisiting if it ever moves to ARG-based image references.

3. Scope boundary — kept exactly as-is, per your confirmation.

pytest tests/unit/test_node_engines_floor.py → 19 passed. The negative control is unchanged: reverting engines to >=20.6.0 still fails exactly the 5 assertions that encode the fix.

Not re-invoking a review yet since the included limit is reached; the push of 339ac79 should pick up an incremental pass when it resets. Note the auto-review also skipped twice here on the label gate — this PR opened with no labels, so it doesn't match the .coderabbit.yaml required-label list.


Generated by Claude Code

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 38a4ea6.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

@groupthinking groupthinking self-assigned this Aug 13, 2026
@groupthinking
groupthinking marked this pull request as ready for review August 13, 2026 07:57
@groupthinking
groupthinking merged commit f6d5ab1 into main Aug 13, 2026
2 of 4 checks passed
@groupthinking
groupthinking deleted the claude/clever-heisenberg-8ktmyz branch August 13, 2026 07:57
@linear-code

linear-code Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

GRV-413

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants