Repository navigation
[Tooling] Align Node minimum to 22.23.4 - #1146
Conversation
Make the pascal doctor Node check follow the engines range from #1142 and update docs/skills that still said Node 22.13 or newer.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Aymericr
left a comment
Aymericr
left a comment
There was a problem hiding this comment.
The isSupportedNodeVersion change and its test look right and match engines.node.
One thing to fix before merge: the docs now say "Node.js 22.23.4 or newer", but the supported range isn't contiguous. 23.x, 24.0–24.22.0, 25.x and 26.0–26.12.0 are "newer" and pascal doctor will now fail them, so a user on Node 24.10 or 25 would read the README as supported and then hit a doctor failure.
Could the doc lines (README.md, SETUP.md, packages/cli/README.md, packages/mcp/README.md, skills/README.md, skills/pascal-3d/SKILL.md, skills/pascal-3d/references/setup.md, skills/furniture-fit/references/setup.md) use the same wording as the doctor message instead, e.g. "Node.js 22.23.4+ on 22.x, 24.22.1+ on 24.x, or 26.12.1+"? Happy to merge once that's in and CI is green.
The supported range is not contiguous, so match the pascal doctor message.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dfc85a27f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| test('node check follows the engines range', () => { | ||
| for (const version of ['22.23.4', '22.24.0', '24.22.1', '24.23.0', '26.12.1', '27.0.0']) { | ||
| expect(isSupportedNodeVersion(version)).toBe(true) |
There was a problem hiding this comment.
This test directly pins the new internal isSupportedNodeVersion helper, which is not exported from src/index.ts; consequently, an equivalent refactor that preserves runDoctor behavior but inlines or replaces the parser breaks the test. T-001 requires tests to use package exports and tolerate equivalent implementations, so exercise the version check through an exported behavior or intentionally make the check part of the package contract.
AGENTS.md reference: AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
| ## Requirements | ||
|
|
||
| - Node.js 22.13 or newer | ||
| - Node.js 22.23.4 or newer |
There was a problem hiding this comment.
For users running Node 23.x or 25.x, “22.23.4 or newer” says their runtime is supported, while both engines.node and the new doctor check explicitly reject those majors; the same misleading shorthand is introduced in the other changed READMEs and skills. List the per-major ranges (22.23.4+ on 22.x, 24.22.1+ on 24.x, or 26.12.1+) and note that 23/25 are unsupported.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fa4da9f. Configure here.
Bun 1.4.4 reports Node 26.3.0, which the engines range rejects, so the packed-runtime smoke test failed running pascal doctor under Bun.
What does this PR do?
Follows up #1142, which raised
engines.nodeto^22.23.4 || ^24.22.1 || >=26.12.1. The remaining places that still said Node 22.13 now match it.Files changed:
packages/cli/src/diagnostics.ts: thepascal doctorNode check now uses the same range asengines(addsisSupportedNodeVersion), so 22.13–22.23.3, 23.x, 24.0–24.22.0, 25.x and 26.0–26.12.0 now fail. Under Bun the check passes and reportsBun <version>: Bun 1.4.4 reports a Node compatibility version of 26.3.0, which does not follow Node security releases and would otherwise fail every Bun launch (the packed-runtime smoke test runspascal doctorunder Bun).packages/cli/src/diagnostics.test.ts: new tests for the version range (just inside and just outside each range) and for the Bun case.README.md,SETUP.md,packages/cli/README.md,packages/mcp/README.md,skills/README.md,skills/pascal-3d/SKILL.md,skills/pascal-3d/references/setup.md,skills/furniture-fit/references/setup.md: "Node.js 22.13 or newer" is now "Node.js 22.23.4+ on 22.x, 24.22.1+ on 24.x, or 26.12.1+", the same wording as the doctor message.How to test
cd packages/cli && bun test src/diagnostics.test.ts: all 5 tests pass.rg '22\.13|22\.23\.4 or newer' --glob '!*.ifc': the only match is the rejected-version case in the new test.Screenshots / screen recording
N/A — non-visual change
Checklist
bun devbun checkto verify)mainbranchNote
Low Risk
Documentation and CLI diagnostic gating only; no runtime behavior change beyond stricter Node version checks in
pascal doctor.Reviewed by Cursor Bugbot for commit f1735c7. Bugbot is set up for automated code reviews on this repo. Configure here.