fix(deps): declare the Node 22 floor the repo already builds on - #1479
Conversation
`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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
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. Comment |
|
@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:
Generated by Claude Code |
🧩 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 || trueLength of output: 45087
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 ✏️ Learnings added
|
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
|
Both findings were correct and are fixed in 1. Union ranges — fixed, and it invalidated a claim in the original PR body. You're right that The part worth calling out: this bug is why the first commit claimed
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 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 2. YAML parsing — fixed. Workflow pins now come from 3. Scope boundary — kept exactly as-is, per your confirmation.
Not re-invoking a review yet since the included limit is reached; the push of Generated by Claude Code |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure 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 FilesNone |
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.nodeadvertised>=20.6.0while nothing in the repo has built or tested on Node 20 for some time. Vercel readsengines.nodeto select the runtime forapps/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.ymlandsecurity.ymlall pinnode-version: 22.apps/web/DockerfilebuildsFROM node:22-slim.>=22foropenai@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 installonly emits anEBADENGINEwarning when a package wants a newer Node than the root declares, so no check ever went red.Scope
package.json—engines.node>=20.6.0→>=22.0.0;engines.npm>=8.0.0→>=10.0.0(Node 22 ships npm 10, so>=8.0.0described 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.chrome-devtools-mcpsits in rootdependenciesrather thandevDependencies.Correction (second commit,
339ac79)The first commit's rationale contained a factual error, caught by CodeRabbit's review and fixed here.
_parse_floorusedre.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 — thatvitest@4.1.10andeslint-visitor-keys@5.0.1"declare>=24". They do not:vitest@4.1.10^20.0.0 || ^22.0.0 || >=24.0.0eslint-visitor-keys@5.0.1^20.19.0 || ^22.13.0 || >=24chrome-devtools-mcp@1.6.0^20.19.0 || ^22.12.0 || >=23vite@8.2.0^20.19.0 || >=22.12.0The 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 returnNone, 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_loadand extracted fromwith["node-version"], matching the precedent intests/unit/test_auto_label_workflow.py. The previous text match would have counted a commented-out# node-version: 20as a pin and failed the test while the workflow still ran Node 22. The Dockerfile check stays a text assertion — it anchors onFROMat line start and so cannot match a comment.Risk
EBADENGINEwarning (a hard failure only underengine-strict, which this repo does not set — root.npmrcsets onlylegacy-peer-deps=true). That surfaces a configuration already broken foropenai@7/ai@7/Supabase rather than introducing a new break. CI, Docker and Vercel all already run Node 22 and are unaffected.enginesreturns to>=20.6.0/>=8.0.0and the new test file goes with it.Verification
Tied to head
339ac79.pytest tests/unit/test_node_engines_floor.py→ 19 passedenginesto>=20.6.0/>=8.0.0fails exactly the 5 assertions that encode the fix (..._matches_the_version_ci_tests_on,..._matches_the_version_the_image_ships, and..._satisfies_core_runtime_dependenciesforopenai,ai,@ai-sdk/gateway); the 14 parser/fixture cases pass either way, as intended>=22 <24, each expected to yieldNoneruff checkclean,black --checkcleanpytest tests/unit/test_logging_config_crlf.py→ 20 passed, confirming fix(security): neutralize CR/LF in rendered log records (CWE-117) #1270/fix(security): neutralize NUL (0x00) in log records (CWE-117 gap from #1270/#1255) #1422/fix(security): build JSON log records with json.dumps (CWE-117 field forgery) #1439 composed correctly339ac79The broader Python suite could not be run in this sandbox:
pytest tests/unit/hits 71 collection errors, allModuleNotFoundError: No module named 'fastapi'and friends. That is a missing-dependency limitation of the environment, not a defect onmain; 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
engines.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
mainbetween 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 stillqueuedat merge time. This PR is deliberately not following that pattern. The repo owner has been notified separately.