fix: use Node 22 in E2E CI and simplify zstd imports#988
Conversation
- Add setup-node@v6 with node-version 22 to E2E test job (Node 20 is EOL) - Replace feature-detected zstd access with direct imports from node:zlib (zstdCompress/zstdDecompress are available in Node 22.15+) - Remove unnecessary runtime fallback code and type gymnastics - Update stale Bun reference in zstd-transport comment
|
![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 and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ced9182. Configure here.
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: oven-sh/setup-bun@v2 | ||
| - uses: oven-sh/setup-bun@v2 |
There was a problem hiding this comment.
E2E workflow YAML step misindented
High Severity
The oven-sh/setup-bun@v2 step is indented one space more than sibling steps under test-e2e steps, so the list item is not aligned with actions/checkout and pnpm/action-setup. GitHub Actions may reject the workflow or treat the Bun setup as nested under checkout instead of a separate step.
Reviewed by Cursor Bugbot for commit ced9182. Configure here.
| ) => Promise<Buffer>) | ||
| : undefined; | ||
| const gzipAsync = promisify(gzipCb); | ||
| const zstdCompressAsync = promisify(zstdCompressCb); |
There was a problem hiding this comment.
Zstd import breaks 22.12 engines
Medium Severity
Replacing runtime zstd feature detection with a static zstdCompress import and unconditional promisify removes the prior gzip fallback. package.json still allows Node >=22.12, but node:zlib only exposes zstdCompress from Node 22.15+, so 22.12–22.14 can fail at module load when this file is imported.
Reviewed by Cursor Bugbot for commit ced9182. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const gzipAsync = promisify(gzipCb); | ||
| const zstdCompressAsync = promisify(zstdCompressCb); |
There was a problem hiding this comment.
Bug: The code unconditionally calls promisify(zstdCompressCb), which is only available in Node.js >=22.15. This will crash on startup for users on Node.js 22.12-22.14.
Severity: CRITICAL
Suggested Fix
Update the BIN_WRAPPER version check in script/bundle.ts to require Node.js version 22.15.0 or greater. Also, consider updating the engines.node field in package.json to " >=22.15" to reflect the new requirement.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/lib/api/sourcemaps.ts#L41-L42
Potential issue: The code unconditionally calls `promisify(zstdCompressCb)` at module
load time. The `zstdCompressCb` function from `node:zlib` was only added in Node.js
v22.15.0. However, the project's `package.json` `engines.node` is set to `>=22.12`, and
the `BIN_WRAPPER` version check was not updated to enforce the new minimum. For users on
Node.js versions 22.12.0 through 22.14.x, `zstdCompressCb` will be `undefined`, causing
`promisify(undefined)` to throw a `TypeError` and crash the CLI on startup.
Also affects:
src/lib/telemetry/zstd-transport.ts:84~85
Did we get this right? 👍 / 👎 to inform future reviews.
## Summary Consolidates fixes for review comments across PRs #967, #970, #984, #986, superseding #988. ### Critical - **Bump `engines.node` to `>=22.15`** — `node:zlib` zstd APIs require 22.15+. Node 20 is EOL. Updated `dist/bin.cjs` runtime version check to match. - **Simplify zstd imports** — replaced feature-detection dance with direct `import { zstdCompress } from "node:zlib"` now that >=22.15 is guaranteed. ### High - **Fix spawn error handling in `upgrade.ts`** — `proc.on("error", () => resolve(1))` discarded the error object, making ENOENT/EBUSY detection dead code. Now properly rejects with the error. ### Medium - **Fix `sqlite.ts` ROLLBACK** — if ROLLBACK throws, the original transaction error was lost. Now wrapped in try/catch. - **Guard `semver.compare`** in `delta-upgrade.ts` with `semver.valid()` to avoid throwing on invalid version strings. - **Narrow catch in `shell.ts`** `addToGitHubPath` — only catch ENOENT, re-throw EACCES/EPERM. - **Add WriteStream backpressure** in `upgrade.ts` `streamDecompressToFile` — check `write()` return value, await `'drain'`. - **Add `setup-node@v6`** with Node 22 to E2E CI job. ### Low - **Fix `whichSync` Windows CRLF** — split on `/\r?\n/` instead of `\n` to strip trailing `\r` from `where.exe` output. ### Test results All 7014 tests pass, 0 failures. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 participant
Summary
Node 20 is EOL. This PR:
Adds
setup-node@v6withnode-version: 22to the E2E test job — the library E2E tests runnode -e "..."with the system Node, which was v20 onubuntu-latest. The npm bundle now requires Node 22+ (node:zlibzstd APIs).Simplifies zstd imports — removes the feature-detection dance that was added in refactor: replace remaining Bun APIs (zstd, mmap, CryptoHasher, file writer) #986 to support Node 20. Now uses direct
import { zstdCompress } from "node:zlib"since Node 22 is the minimum.Removes stale Bun references in comments.
Files changed
.github/workflows/ci.yml— Addsetup-nodeto E2E jobsrc/lib/api/sourcemaps.ts— Direct zstd import, clean promisifysrc/lib/telemetry/zstd-transport.ts— Direct zstd import, clean promisifyAll 7014 tests pass.