fix(sea-builder): make --node's omitted default match its docs - #117
Conversation
normalizeCacheOptions eagerly defaulted nodeVersion to the running machine's own Node version before resolveNodeVersion ever saw the option, so its documented "oldest supported LTS" default was unreachable via the CLI and SEA builds silently embedded whatever Node ran the build instead of a reproducible version. createBuildTasks now resolves the raw, possibly-undefined options.nodeVersion instead of the pre-defaulted value returned by normalizeBuildOptions, and the resolved version is now shown in the "Download the Node.js archives" task title so it's visible in build output. An EOL-aware fallback (skip to the next LTS line if the oldest is already end-of-life) was considered and dropped: all-node-versions exposes no EOL date/status data, so it would need a new dependency or a hand-maintained date table, disproportionate to this fix.
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe SEA builder now resolves omitted ChangesSEA Node.js version handling
IDD source-pinned check policy
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Build
participant Resolver
participant CacheTasks
participant Listr
Build->>Resolver: Resolve raw nodeVersion
Resolver-->>Build: Return supported Node.js version
Build->>CacheTasks: Pass resolved version
CacheTasks->>Listr: Create asynchronous cache tasks
Listr-->>Build: Run cache tasks
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes @kurone-kito/sea-builder’s --node omitted-default behavior so it matches the long-documented contract: when --node is not provided, the build resolves to the latest patch of the oldest supported LTS line (instead of embedding the Node.js version of the machine running the build). It also surfaces the resolved version in the Listr output to improve build reproducibility and debuggability.
Changes:
- Update
createBuildTasksto resolve Node.js version from the raw CLI option (options.nodeVersion) rather than a pre-defaulted normalized value. - Include the resolved Node.js version in the cache task title (
Download the Node.js archives (vX.Y.Z)), and add coverage for both behaviors in new/updated unit tests. - Document the “resolved version is shown in output” detail and record the fix under
CHANGELOG.md→## [Unreleased]→### Fixed.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sea-builder/src/tasks/createCacheTask.spec.mts | Adds a test asserting the cache task title includes the resolved Node version when provided. |
| packages/sea-builder/src/tasks/createCacheTask.mts | Makes the Listr task title include (vX.Y.Z) when opts.nodeVersion is present. |
| packages/sea-builder/src/listr2/createBuildTasks.spec.mts | Adds regression tests to ensure resolveNodeVersion receives the raw (possibly undefined) CLI option and the resolved version is passed to createCacheTask. |
| packages/sea-builder/src/listr2/createBuildTasks.mts | Switches resolution input to options.nodeVersion so the “spec-absent” default path is reachable when --node is omitted. |
| packages/sea-builder/README.md | Notes that the resolved Node.js version is now shown in build output. |
| packages/sea-builder/CHANGELOG.md | Records the fix under ## [Unreleased] → ### Fixed (issue #59). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codex review on #117 found two real gaps in the initial fix: 1. toSemver's spec-absent branch picked the oldest major that ever had an LTS codename, going back through all-node-versions' full history — with no EOL data source, that resolved to Node.js 4.9.1 (end-of-life since 2018) rather than the oldest still-supported LTS. Added the node-releases dependency (release-schedule.json, which does carry end-of-life dates) and a new filterSupportedLts helper that narrows the candidate majors to ones still within their scheduled LTS window before toSemver ever sees them. 2. sea-cache's own omitted-version default still went through normalizeCacheOptions' old process.versions.node fallback, so it could resolve a different archive than sea-builder's newly-fixed default on the same machine, defeating pre-caching. createCacheTasks (and everything that calls it) now resolves nodeVersion the same way createBuildTasks does, so both entry points agree. Verified live (unmocked all-node-versions + node-releases data): resolveNodeVersion() now resolves v22.23.2 (the oldest LTS line still inside its support window as of today), and sea-cache's download URL matches it exactly.
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (4)
packages/sea-builder/src/tasks/createCacheTasks.mts:43
- The JSDoc says this returns a configured Listr instance, but
createCacheTasksreturns aPromise<Task[]>. This is misleading for callers and readers.
* @returns Configured {@link Listr} instance.
packages/sea-builder/src/tasks/createCacheTasks.mts:52
createCacheTasksalways callsresolveNodeVersion, even when the caller already passed an explicit resolved version (e.g. the build pipeline passesvX.Y.Z). This duplicates work and can unexpectedly change behavior ifresolveNodeVersionlogic changes. Consider skipping resolution whennodeVersionalready looks like a fully-resolvedvMAJOR.MINOR.PATCH.
const { cacheDir, download, existsSync, mkdir, nodeVersion, targets } =
normalizeCacheOptions({
...options,
nodeVersion: await resolveNodeVersion(options.nodeVersion),
});
packages/sea-builder/package.json:45
- The PR description/issue decision states an EOL-aware fallback was investigated and dropped due to
all-node-versionslacking EOL status data, but this change adds thenode-releasesdependency (and corresponding logic/tests) specifically to filter out EOL LTS lines. Please either update the PR description/decision to match the implemented approach, or dropnode-releasesand the EOL-filtering behavior to match the documented scope.
"all-node-versions": "^13.0.1",
"execa": "^9.6.0",
"listr2": "^9.0.4",
"node-releases": "^2.0.53",
"semver": "^7.8.1"
packages/sea-builder/src/utils/resolveNodeVersion.spec.mts:35
- This test depends on the real current date because
filterSupportedLtsusesnew Date()by default. As time passes (e.g., when Node 22 reaches its scheduled end date), this expectation will start failing. Freeze the system time within the test to keep it deterministic.
it('resolves latest patch of oldest currently-supported LTS by default, skipping an EOL one', async () => {
await expect(resolveNodeVersion()).resolves.toBe('v22.5.0');
});
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CodeRabbit and Codex found three more issues on #117: - createCacheTasks' own @returns JSDoc still said "Configured Listr instance" (pre-existing drift from before this fix; the function has always returned Promise<Task[]>). Corrected it and dropped the now-fully-unused Listr type import. - resolveNodeVersion's default-resolution test depended on the real wall clock via filterSupportedLts' own now = new Date() default, so it would start failing once Node 22 passes its real 2027-04-30 end date with no code change. Added an injectable now parameter to resolveNodeVersion (threaded through to filterSupportedLts) and pinned it in the existing test; added a companion test proving the default advances once the picked LTS itself ends.
|
Dependency-approval note for CodeRabbit's finding on `node-releases` Adding `node-releases` was explicitly discussed and approved by the repository maintainer (@kurone-kito) before it was added, in the same session that authored this fix — required per this repo's own `CLAUDE.md` guideline ("Ask first: adding/removing dependencies"). Recorded here for a GitHub-visible trail, since the approval itself happened outside a PR comment. Context: the initial fix for #59 (commit 952a7ac's parent) turned out to resolve `--node`'s omitted default to Node.js 4.9.1 (end-of-life since 2018) because `all-node-versions` carries no EOL date data at all. Two options were presented — add `node-releases` (the standard EOL-schedule data source used by e.g. `browserslist`) vs. a hardcoded "top N LTS majors" heuristic — and the maintainer chose the dependency addition as the more correct, less fragile fix. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
packages/sea-builder/src/utils/filterSupportedLts.mts:40
entry.endfrom node-releases is a date-only string (e.g. "2026-04-30"), andnew Date(entry.end)parses to midnight at the start of that day. The current> nowcomparison will treat an LTS line as unsupported for the entire listed end date (off-by-one-day), potentially advancing the default too early.
return Boolean(
entry?.lts && entry.end && new Date(entry.end).getTime() > now.getTime(),
);
packages/sea-builder/src/utils/resolveNodeVersion.mts:22
filterSupportedLts(majors, now)is only relevant for the default (spec omitted) branch intoSemver. Whenspecis provided,toSemverignores the majors list, so this filtering (and the extra date parsing work) is unnecessary on every explicit-version resolution.
const { majors, versions } = await allNodeVersions({ fetch: false });
const range = toSemver(spec, filterSupportedLts(majors, now));
packages/sea-builder/src/tasks/createCacheTasks.mts:51
createCacheTasksnow always callsresolveNodeVersion, even when the caller already provides a fully resolved patch version (e.g.v18.0.0). This forces anall-node-versionslookup (and the node-releases schedule load) on paths that previously had a purely local fast-path, and it also makes unit tests that pass a concrete version unnecessarily integration-heavy.
const { cacheDir, download, existsSync, mkdir, nodeVersion, targets } =
normalizeCacheOptions({
...options,
nodeVersion: await resolveNodeVersion(options.nodeVersion),
});
Copilot found two more real issues on #117: - filterSupportedLts compared a date-only end string (parsed as that day's UTC midnight) directly against now, so the entire scheduled end date itself was treated as already unsupported instead of still-supported through the end of that day. Added a day's worth of milliseconds before comparing. - resolveNodeVersion computed filterSupportedLts unconditionally even when an explicit spec was given, even though toSemver only consults the majors list on its spec-absent branch. Skip the computation entirely when spec is provided. A third suppressed comment (createCacheTasks always calling resolveNodeVersion, even for an already-concrete version) is addressed by the second fix above for the node-releases-schedule half of that cost; the remaining all-node-versions lookup preserves existing validation of explicit versions and is a fast, local, no-network operation, so a further fast-path wasn't pursued.
|
Disposition for Copilot's suppressed comments (review 4904525661, commit 09be49c)
|
This comment has been minimized.
This comment has been minimized.
|
Rejected — coderabbitai[bot] did not review HEAD a7be2a5 (review limit reached / rate limited); this is not a completed review (source: #issuecomment-5249195828) |
This comment has been minimized.
This comment has been minimized.
#51 registered idd-advisory-convergence as a required status check via a repository Ruleset, and the resulting entry turned out to be source-pinned to a specific reporting App (integration_id: 15368, GitHub Actions) rather than a bare check-name match -- the exact condition docs/idd-policy.md already flagged as the trigger to revisit ciGate.trustSourcePinnedRequiredChecks. This PR's own idd-advisory-convergence run is the first one ever evaluated against that ruleset, confirming live that the pinned integration resolves correctly -- discovered while investigating why idd-pre-merge-readiness kept reporting the check as untrusted despite it passing. Explicitly approved by the maintainer before this change, per this repo's own dependency/policy-change approval guideline.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/sea-builder/src/cache.mts:12
mainreturns without awaitingListr#run(). That makes the function contradict its own JSDoc (it won’t wait for completion) and can turn task failures into unhandled promise rejections becauserunIfMaindoesn’t catch them.
export const main = async (...targets: readonly string[]): Promise<void> =>
(await createListrCacheTasks({ targets })).run();
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codex found that filterSupportedLts() returning an empty array (a long-lived installation outlasting node-releases' bundled schedule, or the schedule and all-node-versions temporarily disagreeing) fed toSemver an empty majors list, which resolves to the '*' wildcard range and silently picks the newest available release -- including a non-LTS or already-EOL one -- contradicting the omitted-option contract entirely. resolveNodeVersion now throws a clear, actionable error in that case instead of falling through to the wildcard.
Codex correctly noted that integration_id-pinning only verifies the check was reported by GitHub Actions, not that the workflow content itself is immutable -- a PR can edit idd-advisory-convergence.yml on its own branch to force a trivial pass with the same check name and integration_id. Documented as a known, accepted limitation rather than mitigated: under this repository's fully_autonomous_merge policy, anyone who can open a PR already has direct push access to main, so a "forged check" actor introduces no new attack surface here. Maintainer explicitly confirmed accepting this trade-off.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/sea-builder/src/listr2/createCacheTasks.mts:10
- The JSDoc
@returnsdescription is now inaccurate: createListrCacheTasks is async and returns a Promise, not a Listr instance directly. Update the@returnstext to match the new signature.
* Create Listr tasks for downloading Node.js archives.
* @param options Options controlling the task generation.
* @returns Configured {@link Listr} instance.
*/
export const createListrCacheTasks = async (
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/sea-builder/src/tasks/createCacheTasks.mts:51
createBuildTasksalready resolvesnodeVersionand passes a concretevX.Y.ZintocreateCacheTask, butcreateCacheTasksresolves again unconditionally. This repeats theall-node-versionslookup and schedule filtering even when the version is already fully resolved, adding unnecessary overhead during builds.
const { cacheDir, download, existsSync, mkdir, nodeVersion, targets } =
normalizeCacheOptions({
...options,
nodeVersion: await resolveNodeVersion(options.nodeVersion),
});
Missed this one when converting the function to async earlier in this PR's own review-fix loop -- @returns still said "Configured Listr instance" when it now returns Promise<Listr>.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
F4 Cleanup Evidence (server-side fallback via
|
Summary
Fixes
sea-builder --node's omitted default so it actually matcheswhat
packages/sea-builder/README.mdhas always documented: thelatest patch of the oldest supported LTS line, not whatever Node.js
version happens to run the build.
Root cause
normalizeCacheOptionseagerly defaultednodeVersionto`v${process.versions.node}`beforeresolveNodeVersionever sawthe option, so
toSemver's spec-absent branch (the one that computes^<oldest LTS major>) was unreachable from either CLI —resolveNodeVersionalways received an already-concrete value. SEA builds silently embedded
the Node version of whichever machine ran the build, with nothing in
the output naming the resolved version, making builds non-reproducible
across machines.
Decision
Option (A) — make the code match the docs — chosen on #59 after
weighing trade-offs with the maintainer. See #59 for the full decision
record.
An EOL-aware fallback was initially investigated and dropped as
disproportionate scope, since
all-node-versionsalone exposes no EOLdate/status data. Round 2 (Codex review) proved that call wrong:
without any EOL boundary,
toSemver's spec-absent branch picks theoldest major that ever had an LTS codename in
all-node-versions'full history — verified live, this resolved to Node.js 4.9.1
(end-of-life since 2018), not merely "less polished" but outright
broken. Re-raised with the maintainer, who approved adding
node-releases(the standard EOL-schedule data source, e.g. used bybrowserslist) as a new dependency — see thefilterSupportedLtsaddition below.
Changes
createBuildTasksnow resolves the raw, possibly-undefinedoptions.nodeVersioninstead of the pre-defaulted valuenormalizeBuildOptionsreturns, so omitting--nodereachestoSemver's spec-absent branch again.filterSupportedLtsutility (backed by the newnode-releasesdependency's
release-schedule.json, which carries real EOL dates)narrows the candidate LTS majors to ones still inside their
scheduled support window before
toSemverever sees them.createCacheTasks(and everything that calls it —sea-cache's ownCLI included) now resolves an omitted
nodeVersionthe same waycreateBuildTasksdoes, sosea-builderandsea-cacheagree onwhich archive to fetch by default instead of
sea-cachesilentlykeeping the old
process.versions.nodefallback.createCacheTask's Listr title now includes the resolved Nodeversion (
Download the Node.js archives (vX.Y.Z)) so it's visiblein build output, addressed as recommended in the original issue text
regardless of which option won.
packages/sea-builder/README.md: no change needed — it alreadydocumented the target (fixed) behavior; only the code had drifted
from it. Added one sentence noting the resolved version is now shown
in output.
packages/example-cli'sbuild:seascript: no change needed — itwas already moved off the Node 20 pin in
747792c, before thisissue reached implementation.
packages/sea-builder/CHANGELOG.md: recorded under## [Unreleased].Test plan
createBuildTasks.spec.mts: assertsresolveNodeVersioniscalled with the raw
options.nodeVersion(undefinedwhen--nodeis omitted, not the pre-defaulted value), and that anexplicit
--nodespec passes through untouched.filterSupportedLts.spec.mts: excludes a non-LTS major, anLTS major past its
node-releases-scheduled end date, and along-retired one (Node 4); keeps majors still inside their
window.
resolveNodeVersion.spec.mtsupdated: the default-resolutioncase now includes a mock LTS major (Node 20 "Iron") that's
genuinely past its real-world end date despite
all-node-versionsstill marking it
lts, proving the fix skips it.createCacheTasks.spec.mts/createCacheTask.spec.mtsupdatedfor the new async signature and the title-with-version case.
pnpm run lintclean.pnpm --filter @kurone-kito/sea-builder run test: 59/59 passing,99.3% statement coverage.
pnpm --filter @kurone-kito/sea-builder exec tsc --noEmit: noerrors.
all-node-versions+node-releasesdata, run through vitest:resolveNodeVersion()resolves
v22.23.2(today's oldest still-supported LTS, notNode 4), and
sea-cache's captured download URL(
https://nodejs.org/dist/v22.23.2/node-v22.23.2-linux-x64.tar.gz)matches
sea-builder's own default exactly.Unrelated-looking config change, explained
This PR also flips
ciGate.trustSourcePinnedRequiredCheckstotruein
.github/idd/config.json/docs/idd-policy.md— genuinelyunrelated to the
sea-builder --nodefix itself, but deliberatelybundled here rather than split into its own PR, for a bootstrapping
reason: this very PR is the first one ever evaluated against #51's
newly-registered, source-pinned
idd-advisory-convergencerequiredcheck, and
idd-pre-merge-readinessrefuses to trust a source-pinnedcheck until this flag is enabled — a chicken-and-egg problem for any
standalone PR carrying only that flag flip, since it would hit the
identical "untrusted" gate on itself before it could ever prove the
check is trustworthy. Riding along on a PR whose own
idd-advisory-convergencerun already demonstrably passes (see theTest plan below) is what actually lets this get verified and merged.
Maintainer explicitly approved this coupling before it was made.
Closes #59
Summary by CodeRabbit
--nodeis omitted, builds now select the latest patch of the oldest currently supported Node.js LTS line, excluding end-of-life releases.