Skip to content

chore: exclude Node 23 from the supported engines range - #5291

Open
MasamiYui wants to merge 1 commit into
apache:mainfrom
MasamiYui:chore/node-engines-exclude-23
Open

MasamiYui wants to merge 1 commit into
apache:mainfrom
MasamiYui:chore/node-engines-exclude-23

Conversation

@MasamiYui

Copy link
Copy Markdown
Member

Summary

engines.node was >=22.19.0, which semver-accepts Node 23.x. Node 23 never received node:sqlite DatabaseSync.isTransaction (added in 22.16 / 24.0), so inspectOperationalStateSchema in @maka/storage reads undefined, issues a nested BEGIN, and every State Root open fails with cannot start a transaction within a transaction. npm stays silent because the declared range is satisfied, and CI (22.19.0 / 24) never sees it.

Node 23 is EOL, so this makes the declared support range truthful instead of adding a runtime fallback:

  • package.json / package-lock.json: engines.node^22.19.0 || >=24.0.0
  • scripts/release-cli-package.mjs: validateNodeVersion() rejects major 23 explicitly, with a comment naming the missing API
  • README.md, README.zh-CN.md, CONTRIBUTING.md, CONTRIBUTING.zh-CN.md, packages/cli/README.md, packages/cli/README.zh-CN.md, docs/windows-support.md: requirement wording aligned with the new range

Fixes #5290

Verification

  • node -e "require('semver').satisfies(v, range)" for 22.18.0 ✗, 22.19.0 ✓, 22.20.1 ✓, 23.7.0 ✗, 24.0.0 ✓, 24.18.1 ✓
  • node --test scripts/release-cli-file-policy.test.mjs scripts/release-cli-workflow-policy.test.mjs scripts/release-cli-publication.test.mjs — 44 pass, 0 fail
  • npx biome check package.json scripts/release-cli-package.mjs — clean
  • package.json and package-lock.json still parse; lockfile root engines mirrors package.json
  • Reproduced the failure on Node 23.7.0 before the change (DatabaseSync.isTransaction === undefined, storage suites fail with the nested-transaction error); on Node 22.19+/24 nothing in this PR changes runtime behavior
  • Not run: the full npm test matrix on Node 22 / 24 (no toolchain change for those versions, CI covers them)

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code — root-caused the Node 23 failure, drafted the engines/script/doc edits, and ran the verification above. Commit carries a Generated-by: Claude Code trailer.

Checklist

  • Tests cover the change and fail without it — not applicable: the change is a declared-support range and documentation; the existing release-script policy tests still pass
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above (npm install now warns on Node 23; release:cli:pack refuses Node 23)
  • No

`engines.node` was `>=22.19.0`, which semver-accepts Node 23.x. Node 23 never
received `node:sqlite` `DatabaseSync.isTransaction` (added in 22.16 / 24.0),
so `inspectOperationalStateSchema` in the storage layer reads `undefined`,
issues a nested `BEGIN`, and every State Root open fails with
`cannot start a transaction within a transaction`. npm does not warn because
the range is satisfied.

Tighten the range to `^22.19.0 || >=24.0.0`, make the release packaging
check reject major 23 explicitly, and align the contributor and CLI docs.

Generated-by: Claude Code
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 14, 2026

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed at 0d4f2bf4cf1e1f14ee59b21f8831ba3be211b6cb.

I found no merge-blocking issue. I reproduced the underlying failure with real Node binaries rather than relying on the issue report: both Node 23.7.0 and the final 23.11.1 release lack DatabaseSync.isTransaction, and calling the production Storage schema inspector inside an outer transaction on Node 23.7.0 fails with cannot start a transaction within a transaction. Node 22.19.0 and 24.0.0 expose the required property.

The new engines range rejects Node 23 while preserving 22.19+ on the 22 line and 24+. The release packager now rejects Node 23 before doing any work; the same controlled command on the parent commit passes that guard and reaches a later validation step. The root manifest and lockfile agree, the release manifest inherits that root authority, and the English and Chinese requirements are aligned. Excluding an EOL runtime line is a smaller and clearer repair than adding a Storage fallback for an API gap.

The exact-head full build, the three affected release policy/publication suites (44/44), Biome, and whitespace checks passed. A synthetic merge with current main (85521b122a8605789f9b574fb94e990d480db0ad) is clean; its full build and the same 44 tests also passed. Required hosted checks are successful on this head, including installed CLI validation on Node 22.19 and 24.

One non-blocking test gap remains: no committed test exercises the Node 23 rejection branch or binds the hand-written packager predicate to the root engines range. A small follow-up could extract a pure support predicate and pin the 22.18 / 22.19 / 23.x / 24.0 boundaries.


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@Astro-Han Astro-Han 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.

Review: exclude Node 23 from the supported engines range

Comment-only review. No blocking concerns — the range is correct and the enforcement point is the right one. A few consistency nits below.

Verified

  • Range semantics are correct. ^22.19.0 || >=24.0.0 normalizes to >=22.19.0 <23.0.0||>=24.0.0. Checked with semver: 22.18.0 ✗, 22.19.0 ✓, 22.20.1/22.23.1 ✓, 23.0.0/23.7.0/23.11.0 ✗, 24.0.0/24.18.1 ✓, 25.0.0/26.2.0 ✓. No gap at the 23→24 boundary and no accidental exclusion of the 24+ line.
  • No manifest left behind. The root package.json is the only tracked workspace manifest that declares engines at the base commit (bf6e94), and the lockfile's root entry mirrors it, so package.json:7 + package-lock.json:40 is the complete set.
  • Published CLI manifest picks this up for free. scripts/release-cli-package.mjs:686 sets engines: root.engines in writeReleaseManifest, so maka-agent on npm inherits the narrowed range — no separate edit needed, and npm consumers now get the install-time warning too.
  • No CI matrix allows Node 23. Every node-version pin is 22.19.0, 24, or 24.18.1 (e.g. cli-package-validation.yml:263 node: '22.19.0' / second_node: '24', release-cli-stage.yml:113, ci.yml:195), and the validated matrix in packages/cli/README.md:51 lists only 22.19/24. Nothing to change there.
  • Root cause holds. packages/storage/src/operational-state-store.ts:356 guards on if (database.isTransaction) before issuing its own BEGIN; on Node 23 that property is undefined (falsy), so the nested BEGIN and the reported failure follow directly.
  • Guard change is sound and test-safe. validateNodeVersion() (scripts/release-cli-package.mjs:263-267) now rejects major 23 while still accepting 22.19+ and 24+, matching engines. I grepped scripts/release-cli-*.test.mjs — nothing asserts on the old Node.js >=22.19.0 is required message, so the message change shouldn't break the policy suites.
  • Docs were updated in both en and zh-CN for README, CONTRIBUTING, the CLI README, and docs/windows-support.md, and the prose stays accurate to the range.

Nits

  1. website/src/copy/en.ts:226 / website/src/copy/zh-CN.ts:204 still read Node.js 22.19 or newer / Node.js 22.19 或更高版本 in the "Build from source" prerequisites. That is the same imprecise claim the PR tightens in README.md:67, and the website is now the one user-facing surface that still allows Node 23 by implication. Cheap to align in the same PR.
  2. docs/runtime-host-remote-access.md:28 and .zh-CN.md:28 — "On a machine with Node.js 22.19 or newer …". Same wording class; optional.
  3. docs/cli-npm-release.md:211 (and .zh-CN.md:188) says "Node.js 22.14.0 or newer", which predates even the old 22.19.0 baseline. Pre-existing, out of scope here, but it's the last place in docs/ with a different floor.
  4. Two hand-maintained sources of truth. engines and the explicit major === 23 clause must be kept in sync manually; a future major (say 27) losing the same API would need a third edit. Deriving the check from the root manifest (semver.satisfies(process.versions.node, rootManifest.engines.node)) would remove the drift risk — noting that semver is only a transitive dep today, not declared in the root manifest, so that would be a slightly larger change. Fine as-is for this PR.
  5. Enforcement is advisory at install time. There's no .npmrc with engine-strict anywhere in the repo, so on Node 23 npm install warns but does not fail; the actual gate is release:cli:pack. Matches the behavior-change note in the PR body — just worth being explicit in the docs that the release script is the enforcing layer.

Not checked

I did not run npm test; per the PR body nothing in the 22.19+/24 paths changes behavior, and CI covers those.

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

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node 23 satisfies engines >=22.19.0 but lacks node:sqlite isTransaction; every State Root open fails

3 participants