Skip to content

fix(cli): stop with a clear message on a Node older than engines.node - #689

Merged
theCodeDrift merged 3 commits into
mainfrom
fix/node-version-message
Nov 12, 2026
Merged

theCodeDrift merged 3 commits into
mainfrom
fix/node-version-message

Conversation

@theCodeDrift

Copy link
Copy Markdown
Member

Fail with a clear message on a Node older than engines.node (>=22.22.0), via npx and via the project launcher.

  • npx / pnpm dlx: package bin now points at a small dist/bin.js (src/bin.ts + src/node-version.ts, engines.node injected at build) that checks process.versions.node and only then import()s the CLI. On an older Node: taskless: Taskless needs Node >=22.22.0, you have 20.19.0. Install a newer Node and run it again., exit 1. The built file imports only node:process. dist/index.js keeps its shebang, so pnpm cli, CI and the self build are unchanged.
  • Launcher (assets/launcher/launcher.mjs): same check from the lockfile's engines.node before npm ci, and from the installed package.json before starting a cached version.
  • agent/ci.md (topic v11): says "22.22 or later" and that an older CI Node must be raised.
  • OpenSpec node-version-guard, archived in the second commit.

Notes: only >= ranges are understood; anything else is allowed through. Not run on a real Node 20 (tests override process.versions.node). The "CI docs page showing Node 20" isn't in this repo (likely docs.taskless.io). Existing projects get the launcher check when init rewrites their launcher.

Tests: node-version.test.ts (ranges, plus the built bin.js on a faked 20.19.0) and three launcher cases. typecheck, lint and the targeted suites pass.

Commits are unsigned: the signing key couldn't be unlocked remotely, and the owner chose to ship unsigned.

Closes #679

@theCodeDrift

Copy link
Copy Markdown
Member Author

@claude /review

@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @theCodeDrift's task in 4m 17s —— View job


Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained only the trigger comment itself (no prior reviews, review threads, or other top-level comments), so this is the first review of this PR.

  • Read prior review data (.prior-review.json)
  • Fetch PR diff and metadata
  • Read changed files for context (node-version.ts, bin.ts, launcher.mjs, vite.config.ts, install-lockfile.ts, package.json, ci.md, tests)
  • Review correctness, security, performance, tests, clarity
  • Post inline comments for concrete issues
  • Post final summary

Assessment

This is a well-scoped, well-tested fix. I traced the full path for both entry points:

  • npx/pnpm dlx (src/bin.ts + src/node-version.ts): the dynamic import("./index") correctly avoids linking the CLI's module graph (and thus the globSync failure) before the version check runs. nodeVersionProblem's numeric major/minor/patch comparison is correct, including the boundary case (>=22.22.0 vs exactly 22.22.0 is allowed) and ranges it deliberately doesn't understand (anything but a bare >= lower bound passes through untouched).
  • Launcher (assets/launcher/launcher.mjs): requireNode is called at both the right points — from the lockfile's recorded engines before npm ci, and from the installed version's own package.json after resolving (or skipping) the install — and installVersion's existing lock/version check runs first, so the new check only sees a locked entry that's already known to match.
  • I checked the one place this could have silently been a no-op in production: whether engines actually survives into the launcher's embedded npm lockfile. It does — scripts/install-lockfile.ts:191 already threads locked.engines into the node_modules/<pkg> entry, and it's passed through from vite.config.ts's installLockfile(). So the pre-install check isn't just exercised by hand-built test fixtures; it's wired for real builds too.
  • Build wiring (vite.config.ts) adding bin as a second PROGRAM_ENTRIES member (shebang + chmod) and the __TASKLESS_NODE_ENGINE__ define both look correct, and the new entry doesn't trip assertExportsClassified/assertLibraryGraphs since bin isn't a published subpath export.
  • Test coverage is solid: boundary conditions, unparseable ranges, the "already installed under a newer Node, run under an older one" case, and the built bin.js spawned directly with a faked process.versions.node. The PR description's own caveat (not exercised against a real old Node interpreter, only a faked process.versions.node on the current one) is an honest, disclosed limitation rather than a hidden gap.

One [New] low-severity note posted inline on packages/cli/src/node-version.ts: the comparison logic and message text are duplicated by hand in launcher.mjs (necessarily, since that file ships standalone into user projects and can't import this module), but nothing currently enforces the two stay in sync. Not a bug — I checked both against all cases in the two test files and they agree — just a drift risk worth a cross-reference or shared test fixture.

No correctness, security, or performance issues found.

Comment thread packages/cli/src/node-version.ts Resolved
@theCodeDrift

Copy link
Copy Markdown
Member Author

Re: @github-actions — "Claude finished @theCodeDrift's task in 4m 17s — Review complete…"
#689 (comment)

Thanks for the review. The one inline note (the duplicated Node check) now has a cross-reference in both files, in 4bf5daa. That commit also fixes the failing Validate check: nightly-pack.test.cjs still expected bin: ./dist/index.js after this PR moved it to ./dist/bin.js.

— AI Coding Agent

… two Node checks

The bin moved from dist/index.js to dist/bin.js, and the nightly-pack test
still asserted the old path. Also note in both node-version.ts and the
launcher that their version comparisons mirror each other.
@theCodeDrift
theCodeDrift force-pushed the fix/node-version-message branch from 4bf5daa to f11d174 Compare November 12, 2026 04:25
@theCodeDrift
theCodeDrift merged commit 7dd9cff into main Nov 12, 2026
4 checks passed
@theCodeDrift
theCodeDrift deleted the fix/node-version-message branch November 12, 2026 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fail with a clear Node version message on Node < 22.22, via the launcher and via npx

1 participant