Skip to content

chore: use Node 24 LTS for binary builds + explicit tree shaking #1018

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BYK merged 1 commit into from
May 23, 2026
+18 −6
Merged

chore: use Node 24 LTS for binary builds + explicit tree shaking #1018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion .lore.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,9 @@
<!-- lore:019e51e9-4b16-72c5-8733-1a441a0a107d -->
* **Always monitor CI after push and fix all failures before considering work done**: After pushing code or merging PRs, the user expects the assistant to actively monitor CI results, wait for all checks (including bots like Sentry Seer and Cursor BugBot) to complete, fix any failing jobs, and address all unresolved comments (from both bots and humans). The cycle repeats until CI is fully green and no unresolved comments remain. Use \`gh run view --log-failed\` and \`gh pr checks\` to identify failures. Do not consider a task complete until this full cycle is done.

<!-- lore:019e54cf-7a3a-7205-bd62-dce06d6ee5b1 -->
* **Always pause current tasks to resolve architectural blockers before implementation**: When the user discovers that a foundational architectural assumption is wrong or a planned approach has a critical flaw (e.g., a security boundary that doesn't hold, a rejected integration pattern), they immediately reprioritize: they pause the current implementation task and require the architecture to be redesigned first before any further implementation proceeds. This applies even mid-task. The user explicitly calls out the rejected approach and states the new design direction, expecting the assistant to treat the architectural decision as a prerequisite gate before resuming the original work.

<!-- lore:019e5354-d2f6-776c-96b5-1a6ff09a4050 -->
* **Always provide documentation/context dumps before requesting technical analysis**: The user consistently pastes large reference documents, source files, or full code listings into the conversation before asking for analysis or implementation work. This applies when exploring new APIs (e.g., Node.js SEA docs), auditing codebases, or planning migrations. The user expects the assistant to extract key insights, identify problems, and propose solutions directly from the pasted material — not from general knowledge alone. When responding, prioritize findings grounded in the specific pasted content, cite line numbers or section names where possible, and proactively surface implications the user may not have explicitly asked about.

Expand All @@ -201,6 +204,9 @@
<!-- lore:019e5276-c2e3-7f64-a545-8dc3a65798bf -->
* **Always pursue native runner builds to enable platform-specific optimizations**: When the user discovers that cross-compilation from a non-native runner is blocking optimizations (e.g., code cache, smoke testing, codesigning), they consistently push to move builds to native runners for each target platform. This applies to macOS targets moving from ubuntu-latest to macos-latest, and extends to investigating per-platform features like useCodeCache and useSnapshot in fossilize. When the assistant identifies a cross-compilation limitation, the user expects a concrete plan to restructure CI matrix jobs to use native runners, and will follow up to explore related optimizations (e.g., cloning upstream repos to check feasibility). Always check whether current CI runners match the target platform and propose native runner alternatives when they don't.

<!-- lore:019e5577-3833-7c19-bacc-64bd23fdd0b5 -->
* **Always request a critical pre-merge review before merging PRs to production**: Before merging any PR, the user consistently requests a thorough, final pre-merge review — often using a subagent for objectivity. The review follows a structured checklist covering: file corruption (especially from biome formatter), import consistency (.js extensions, unused/missing imports), type safety, PR description accuracy, dead code, security issues (e.g., shell injection), error handling correctness, test coverage, and lint/CI status. The user expects the reviewer to surface BLOCKING vs NON-BLOCKING findings explicitly, and only approves merge when zero blocking issues are confirmed. This pattern applies to all PRs regardless of size or prior review rounds.

<!-- lore:019e51ed-de34-73e5-9b09-56df6f0ebbc0 -->
* **Always update dependencies promptly after releasing new versions**: When the user releases a new version of a tool they own (e.g., fossilize), they immediately update dependent projects to use that new version. This includes bumping the version in package files, creating a dedicated branch with a descriptive name (e.g., \`chore/tool-x.y.z\`), and opening a pull request. The commit message follows conventional commit format: \`chore: update \<tool> to \<version> (\<key feature>)\`. The assistant should proactively handle the full update workflow: fetch latest main, create the branch, update the dependency, commit, push, and open a PR.

Expand All @@ -222,8 +228,11 @@
<!-- lore:019e5026-1e77-7046-a0cc-9251e57550de -->
* **Prefers Bun-native APIs; use buildCommand from lib/command.js (never @stricli/core directly); use buildRouteMap from lib/route-map.js; silent catch blocks prohibited; every new src/lib/\*\*/\*.ts must start with module-level JSDoc; test isolation via useTestConfigDir(); prefer property-based and model-based tests over unit tests; DEFAULT\_NUM\_RUNS = 50; architecture tree documented; error exit code ranges: 1x=auth**: Project conventions (AGENTS.md): use \`pnpm run\`/\`pnpm install\`/\`pnpm add -D\` (NOT bun for package management); use buildCommand from lib/command.js (never @stricli/core directly); use buildRouteMap from lib/route-map.js; silent catch blocks prohibited; every new src/lib/\*\*/\*.ts must start with module-level JSDoc; test isolation via useTestConfigDir(); prefer property-based and model-based tests over unit tests; DEFAULT\_NUM\_RUNS=50; error exit code ranges: 1x=auth, 2x=input/config, 3x=API/network, 4x=feature/billing, 5x=operations, 6x=command-specific. Testing: vitest + fast-check. All packages in devDependencies (CI enforces via \`pnpm run check:deps\`). NEVER merge a PR if CI is failing unless explicitly told to ignore. Always use \`pnpm add -D \<package>\` — never add to \`dependencies\`.

<!-- lore:019e54bc-cdcf-7374-bc5b-d80aaed508bc -->
* **Respect explicitly rejected approaches**: Behavioral pattern detected across 3 sessions (action: rejected-approach). The user consistently demonstrates this behavior.

<!-- lore:019e5487-bc46-7356-93c7-ef2608b13707 -->
* **Review code before committing**: Behavioral pattern detected across 4 sessions (action: requested-review). The user consistently demonstrates this behavior.
* **Review code before committing**: Behavioral pattern detected across 5 sessions (action: requested-review). The user consistently demonstrates this behavior.

<!-- lore:019e51ac-82f5-7281-9e98-1d5964d8e931 -->
* **Smoke tests must cover critical lazy-loaded paths, not just --help/--version**: Smoke tests that only run \`--help\` are insufficient — they never trigger lazy-loaded code paths. Critical paths: \`auth status\` (exits code 10, \`auth: false\`, exercises SQLite init/schema migrations/telemetry lazy import/CJS require chain, no network calls) and \`cli defaults\` (exits 0, \`auth: false\`, exercises \`getAllDefaults()\`/metadata KV). Both binary and npm bundle smoke tests must cover these paths. \`init --dry-run\` is NOT suitable as a smoke test — it lacks \`auth: false\`, so the auth guard runs first. CI currently only runs \`--help\` for all smoke tests (ci.yml lines 277-285, 683).
Expand Down
10 changes: 6 additions & 4 deletions script/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ const pkg = JSON.parse(await readFile("package.json", "utf-8"));
const VERSION: string = pkg.version;

/** Pin to Node 22 LTS for SEA binaries */
const NODE_VERSION = "22";
/** Node version for SEA binaries. "lts" resolves to the latest LTS via fossilize. */
const NODE_VERSION = "lts";

/** Files that use _require() for lazy relative imports (circular dep breaking). */
const REQUIRE_ALIAS_FILTER =
Expand Down Expand Up @@ -116,10 +117,11 @@ async function bundleJs(): Promise<boolean> {
bundle: true,
outfile: BUNDLE_JS,
platform: "node",
// Target Node 22 to downlevel `using` declarations (not supported
// in CJS). Node SEA runs embedded JS as CJS.
target: "node22",
// Target Node 24 LTS. Downlevels `using` declarations (not
// supported in CJS). Node SEA runs embedded JS as CJS.
target: "node24",
format: "cjs",
treeShaking: true,
// Externalize the Ink + React stack from the esbuild bundling
// step. The main bundle never calls `import("ink")` at runtime —
// the sidecar is pre-bundled by text-import-plugin as a
Expand Down
3 changes: 2 additions & 1 deletion script/bundle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,11 +189,12 @@
entryPoints: ["./src/index.ts"],
bundle: true,
minify: true,
treeShaking: true,
// No banner — warning suppression moved to dist/bin.cjs (CLI-only).
// The library bundle must not suppress the host application's warnings.
sourcemap: true,
platform: "node",
target: "node22",
target: "node24",

Check warning on line 197 in script/bundle.ts

View check run for this annotation

@sentry/warden / warden: find-bugs

Library bundle targets node24 but package engines allows Node 22 

Setting `target: "node24"` in the npm library bundle (`dist/index.cjs`) tells esbuild to skip downleveling any syntax that Node 24 supports natively; if any such syntax is used in source or dependencies, it will be emitted verbatim and break consumers running Node 22 (permitted by `engines.node >= 22.15`).
Copy link
Copy Markdown

@sentry-warden sentry-warden Bot May 23, 2026

Choose a reason for hiding this comment

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

Library bundle targets node24 but package engines allows Node 22

Setting target: "node24" in the npm library bundle (dist/index.cjs) tells esbuild to skip downleveling any syntax that Node 24 supports natively; if any such syntax is used in source or dependencies, it will be emitted verbatim and break consumers running Node 22 (permitted by engines.node >= 22.15).

Evidence
  • bundle.ts line 197 changes target from "node22" to "node24" for the output dist/index.cjs, which is the npm library entry point.
  • package.json engines.node remains ">=22.15", explicitly allowing Node 22 consumers of the npm package.
  • esbuild's target determines which syntax transformations are skipped; a node24-only syntax feature in source or a bundled dep will pass through untransformed.
  • The binary build (build.ts) also uses node24 but embeds its own Node 24 runtime (via fossilize), so that path is safe — the library bundle path is not.
  • The PR description acknowledges the binary embeds Node 24, but does not address this library-bundle target mismatch.

Suggested fix: Keep the library bundle target aligned with the minimum supported engine version so esbuild correctly downlevels any syntax gaps.

Suggested change
target: "node24",
target: "node22",

Identified by Warden find-bugs · 89U-MM4

format: "cjs",
outfile: "./dist/index.cjs",
// Inject Bun polyfills and import.meta.url shim for CJS compatibility
Expand Down
Loading