Skip to content

chore(build): rename configs, remove dead code, fix runtime bugs#1140

Open
jdalton wants to merge 3 commits intomainfrom
chore/build-cleanup
Open

chore(build): rename configs, remove dead code, fix runtime bugs#1140
jdalton wants to merge 3 commits intomainfrom
chore/build-cleanup

Conversation

@jdalton
Copy link
Copy Markdown
Contributor

@jdalton jdalton commented Mar 31, 2026

Summary

Config renames (consistent naming scheme):

  • esbuild.cli.build.mjsesbuild.cli.mjs
  • esbuild.index.config.mjsesbuild.index.mjs
  • esbuild.config.mjsesbuild.build.mjs
  • esbuild-shared.mjsesbuild-utils.mjs

Dead code removed:

  • esbuild-plugin-dead-code-elimination.mjs (unused plugin)
  • scripts/esbuild.config.mjs (broken orphan importing non-existent file)
  • socketPackages={} + resolvePackageSubpath + resolve-socket-packages plugin (~50 lines never executed)
  • Template esbuild.build.mjs files (never called by any build script)
  • INLINED_LEGACY_BUILD constant (no references)
  • build:sea:internal:bootstrap, publish:sea, e2e:smol scripts (all referenced missing files)
  • treeShaking: true and external: [] (redundant esbuild defaults)

Refactored:

  • createBuildRunnerrunBuild (static imports, no IIFE, rethrows on failure for Promise.allSettled)
  • esbuild.build.mjs runs builds in-process via runBuild instead of spawning subprocesses
  • Extracted resolveSocketLibExternal() and resolveConstant() helpers (deduped repeated blocks)

Runtime bugs fixed:

  • runBuild() now rethrows so Promise.allSettled rejection detection works correctly
  • download-assets.mjs: allSettled handles both rejected promises and ok: false fulfilled results
  • esbuild.cli.mjs: plugin order fixed — unicodeTransformPlugin runs before envVarReplacementPlugin
  • build.mjs watch mode: error message uses watchResult?.code ?? 1 fallback
  • test-wrapper.mjs: spawn() result used with .on() (Promise, not ChildProcess)
  • validateBundle(): mixed false|array return type fixed
  • verify-package.mjs: undefined fn ref, logger scope, forbidden fs.access(), unnecessary process import
  • wasm.mjs, patches.mjs: logger out of scope
  • validate-bundle.mjs: console.error → logger
  • package.json: e2e:sea indentation fixed

CI: Updated GitHub Actions install action pins in ci.yml, provenance.yml, weekly-update.yml

Consistency: Promise.allPromise.allSettled for parallel tasks, utf8utf-8, console.* → logger, stale JSDoc removed

Test plan

  • pnpm run build completes without errors
  • pnpm run build --force rebuilds cleanly
  • SEA binary builds successfully
  • CI passes

Note

Medium Risk
Touches the CLI build/publish pipeline (esbuild orchestration, template builds, and CI install action pins), so regressions could break packaging even though changes are mostly renames, dead-code removal, and error-handling fixes.

Overview
Streamlines the CLI build system by renaming/standardizing esbuild config entrypoints and replacing the old subprocess-based orchestrator with an in-process esbuild.build.mjs runner that calls the new runBuild() utility.

Removes unused/broken build code (the dead-code-elimination esbuild plugin export/implementation, legacy esbuild scripts/configs, unused env constants, and several stale package.json scripts) and updates package-builder templates to reference the renamed configs.

Hardens build/runtime scripts with more reliable spawning and parallel execution (Promise.allSettled, null spawn checks, consistent utf-8 reads, logger usage), and updates docs/CI workflows to match (including a new pin for the shared SocketDev/socket-registry install action).

Written by Cursor Bugbot for commit 51d1285. Configure here.

@jdalton jdalton requested a review from Copilot March 31, 2026 23:57
@jdalton

This comment was marked as outdated.

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

jdalton added 3 commits March 31, 2026 22:44
Renames:
- esbuild.cli.build.mjs → esbuild.cli.mjs
- esbuild.index.config.mjs → esbuild.index.mjs
- esbuild.config.mjs → esbuild.build.mjs
- esbuild-shared.mjs → esbuild-utils.mjs

Dead code removed:
- esbuild-plugin-dead-code-elimination.mjs (unused plugin)
- scripts/esbuild.config.mjs (broken orphan, imported non-existent file)
- socketPackages={} + resolvePackageSubpath + resolve-socket-packages plugin (~50 lines never executed)
- template esbuild.build.mjs files (never called by any build script)
- INLINED_LEGACY_BUILD constant (no references in codebase)
- build:sea:internal:bootstrap script (referenced missing file)
- publish:sea script (referenced missing publish-sea.mjs)
- e2e:smol script (--smol not in BINARY_FLAGS, always errored)
- treeShaking:true (redundant default when bundle:true)
- external:[] (redundant default with platform:'node')

Refactored:
- createBuildRunner → runBuild (static imports, no IIFE, no dynamic imports)
- esbuild.build.mjs runs builds in-process via runBuild instead of spawning subprocesses
- Extracted resolveSocketLibExternal() helper (deduped 2x verbatim block)
- Extracted resolveConstant() inner fn (deduped 2x identical constants resolver)
- All template configs use runBuild() instead of duplicating manual build+write pattern

Runtime bugs fixed:
- test-wrapper.mjs: spawn() result used with .on() event listeners (Promise, not ChildProcess)
- validateBundle(): returned false|array mixed type; caller did violations.length on false
- verify-package.mjs: error(err) called undefined fn; logger used outside validate() scope;
  fileExists() used forbidden fs.access(); unnecessary import process from node:process
- wasm.mjs: showHelp() used logger not in scope (only defined in error branch)
- patches.mjs: commitPatch() used logger not in scope; local logger shadowed module import
- build.mjs watch mode: watchResult.code accessed without null guard
- build-js.mjs: buildResult.code accessed without null guard
- cli-sentry template build.mjs: two spawn results accessed .code without null guard
- validate-bundle.mjs: .catch used console.error instead of logger

Consistency:
- All esbuild configs use sourcemap:false explicitly
- utf8 → utf-8 throughout
- All console.log/error → logger across .config/ and scripts/
- Stale JSDoc removed from cover.mjs helpers
- build-infra/package.json and cli/package.json indentation fixed

chore(build): Promise.allSettled, fix import order, update stale docs

- Switch Promise.all → Promise.allSettled in build.mjs phase 4 post-processing,
  esbuild.build.mjs variant orchestration, and download-assets.mjs parallel
  downloads so all tasks complete before reporting failures
- Fix import order in esbuild.build.mjs (const logger was between import groups)
- docs/build-guide.md: update config file names (esbuild.cli.mjs, esbuild.index.mjs,
  esbuild.build.mjs) to match renames from previous commit
- build-infra/README.md: remove deleted deadCodeEliminationPlugin section and
  stale usage examples; update Related Files to current config names

chore(build): Promise.allSettled in download-iocraft-binaries
- runBuild() rethrows after catching so Promise.allSettled can detect rejections
- download-assets: handle rejected promises in allSettled, not just ok:false
- esbuild.cli.mjs: fix plugin order so envVarReplacement runs after unicode transform
- build.mjs: fix watch mode error message to use fallback exit code 1
- package.json: fix e2e:sea indentation
@jdalton
Copy link
Copy Markdown
Contributor Author

jdalton commented Apr 1, 2026

@cursor review

@jdalton jdalton force-pushed the chore/build-cleanup branch from 51d1285 to 8491dd3 Compare April 1, 2026 03:18
@jdalton jdalton enabled auto-merge (squash) April 1, 2026 03:19
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Rejected promises silently ignored after allSettled migration
    • Added r.status === 'rejected' to the failed filter and proper error message handling for rejected results, matching the pattern already used in download-assets.mjs.

Create PR

Or push these changes by commenting:

@cursor push 4dccb8034b
Preview (4dccb8034b)
diff --git a/scripts/download-iocraft-binaries.mjs b/scripts/download-iocraft-binaries.mjs
--- a/scripts/download-iocraft-binaries.mjs
+++ b/scripts/download-iocraft-binaries.mjs
@@ -159,12 +159,14 @@
   )
 
   const failed = settled.filter(
-    r => r.status === 'fulfilled' && !r.value.ok,
+    r => r.status === 'rejected' || (r.status === 'fulfilled' && !r.value.ok),
   )
   if (failed.length > 0) {
     logger.error(`\n${failed.length} platform(s) failed:`)
     for (const r of failed) {
-      logger.error(`  - ${r.value.target}`)
+      logger.error(
+        `  - ${r.status === 'rejected' ? r.reason?.message ?? r.reason : r.value.target}`,
+      )
     }
     return false
   }

You can send follow-ups to this agent here.

Comment @cursor review or bugbot run to trigger another review on this PR

const failed = results.filter(r => !r.ok)
const failed = settled.filter(
r => r.status === 'fulfilled' && !r.value.ok,
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rejected promises silently ignored after allSettled migration

High Severity

The failed filter only checks r.status === 'fulfilled' && !r.value.ok, completely ignoring r.status === 'rejected' results. If downloadIocraftBinary throws an uncaught error, Promise.allSettled captures it as a rejected entry, but this filter skips it. The function then incorrectly reports "All N platform(s) downloaded successfully" and returns true. The same Promise.allPromise.allSettled migration in download-assets.mjs correctly includes r.status === 'rejected' in its filter, making this an inconsistency.

Fix in Cursor Fix in Web

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.

2 participants