Repository navigation
feat(root): support Node.js 26 and test Node.js 27 - #10043
zahin-mohammad wants to merge 6 commits into
Conversation
zahin-mohammad
changed the title
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. A human should still look, because it changes CI merge-gating policy and swaps a resolved dependency in yarn.lock, and CODEOWNERS assigns both to owning teams.
What was reviewed:
- CI matrix: 26.x is now required. The 27-nightly leg is non-blocking through job-level
continue-on-error, so a setup or install failure there shouldn't failall-checks. Addingci.ymlto thesourcefilter means workflow-only edits now run unit tests. yarn.lockmovesyargsfrom 16.2.0 to 16.2.2 within the existing^16.2.0range. I couldn't check the new version and integrity hash against the npm registry from this environment, so that's worth confirming.- Engine ranges: both coin generators now use
>=22, which matches the updated README and DEVELOPERS.md. The rest of the diff is Prettier reformatting and wording in CLAUDE.md and the READMEs.
Extended reasoning...
The change touches the CI workflow (which Node.js versions must pass before merge), one transitive dependency resolution in yarn.lock, the engine ranges in both coin-package generators, and docs. No runtime SDK code, authentication or cryptographic paths change. The CODEOWNERS file assigns the root, /scripts/ and /yarn.lock to specific teams. The yargs lockfile bump is a supply-chain change whose registry metadata couldn't be checked from here. Together, these make a human review worthwhile even though the diff is small.
8966ebc to
3ae2591
Compare
|
|
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
.github/workflows/ci.yml— A hang on the Node.js 27 nightly leg can still block every pull request's merge, despite the non-blocking setup. Step-levelcontinue-on-erroronly absorbs a step that exits with failure. Theunit-testjob has notimeout-minutes, so a hung step runs until GitHub's 360-minute job limit. The job then ends as failed or cancelled, andall-checksrequiressuccessorskipped. Fix: settimeout-minuteson the experimentalUnit Testandbuild packagessteps (for example 60), well under the job limit, so a hang becomes a step failure thatcontinue-on-errorabsorbs and the warning comment reports.Why this was flagged
Trigger: a Node.js 27 nightly regression makes mocha or tsc under
yarn run unit-test-changedoryarn run postinstallhang instead of exiting non-zero. An open handle that keeps the event loop alive, or a deadlock in the nightly runtime, would do it; I cannot reproduce this from the checkout. Theunit-testjob (.github/workflows/ci.yml:55) sets notimeout-minutesat job or step level. Itscontinue-on-error: ${{ matrix.experimental }}(ci.yml:141, :133) only applies when a step finishes with a failure. A job hitting the 360-minute default ends as failed or cancelled, andall-checks(ci.yml:539-556) passes only when everyneedsresult issuccessorskipped, so merge is blocked. The base had the same gap on a released 26.x Current line. This change moves the experimental leg to master nightlies, which are much more likely to regress. The description says every 27 step yields a green check plus a warning, and that does not hold for a hang. A step timeout turns the hang into a step failure, which the existingcontinue-on-errorand failure-comment logic already handle.Verification: The
unit-testjob (ci.yml:55-61) has notimeout-minutes. The experimental steps only getcontinue-on-error: ${{ matrix.experimental }}(ci.yml:133, :141). A step that never exits runs until GitHub's 6-hour job limit and ends cancelled or failed.all-checksthen fails at ci.yml:556, which blocks merge. The base had the same gap for its experimental 26.x leg.
|
Addressed the remaining Node.js 27 nightly review findings in a36d0d4: setup now times out after 15 minutes, while install, build, and unit-test steps time out after 60 minutes. Because these experimental steps retain continue-on-error, a timeout becomes a reported compatibility warning instead of waiting for GitHub’s six-hour job limit. The same commit prevents failed installs from being cached and requires an actual successful unit-test outcome before resolving the warning. Validation: actionlint v1.7.7 and Prettier both pass for .github/workflows/ci.yml. |
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
3235ff7 to
171091b
Compare
171091b to
dd0f0d9
Compare
dd0f0d9 to
1f26128
Compare
6256756 to
9696afe
Compare
Document the Node.js 24 container baseline. Align source prerequisites with the supported Node.js 22, 24, and 26 releases. Ticket: WCN-3753
Limit repository-wide Prettier checks to eight processes and ESLint checks to two processes. This avoids V8 crashes and runner resource exhaustion. Ticket: WCN-3753
Use the current wallet signing method in mocks and assert exact expected responses. Unexpected missing-method errors now fail instead of satisfying broad error checks. Ticket: WCN-3753
ec01191
9696afe to
ec01191
Compare
Summary
yargscommand-line parser dependency to 16.2.2, which fixes Mocha startup on Node.js 26 by using an explicit CommonJS entry point.>=22, which includes Node.js 26.master, and document that it tracks the older of the two most recent Long-Term Support (LTS) releases. Express source installs support Node.js 22, 24, and 26.The default
.nvmrcdevelopment runtime remains Node.js 24 until Node.js 26 reaches LTS, following the repository upgrade policy. The Node.js 26 CI leg is intentionally blocking before that transition.Validation
yarn install --frozen-lockfile --ignore-scripts --registry https://registry.npmjs.orgyarn unit-test-changedon Node.js v26.8.1: passed forbitgoand@bitgo/express; 1,978 Express tests passedyarn check-versionsyarn commitlint --from=origin/master -Vgo run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 .github/workflows/ci.ymlyarn prettier --check .github/workflows/ci.yml modules/express/README.mddocker build --check --platform=linux/amd64 .scripts/sdk-coin-generator-v2/core/generator.tsTicket
WCN-3753: Add Node.js 26 support to BitGoJS