From aa9010d1603271178c1015a607a8c9828a0a1d4d Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Mon, 3 Aug 2026 13:41:23 -0400 Subject: [PATCH 1/5] fix(instrument-bundler): keep the esbuild switch alive through tree shaking Importing instruments from a GitHub repo failed for every instrument in any production build, while working under `pnpm dev`. The API reported `Imported 0 instruments`, and each instrument logged a bare `InstrumentBundlerError: Failed to Compile`. `src/vendor/esbuild.ts` initialized its exports from a detached statement: if (typeof window === 'undefined') { var { build, transform } = await import('esbuild'); } else { ... } export { build, transform }; `package.json` declares `sideEffects: ['**/cli.ts']`, advertising every other file in this package as side-effect free, so esbuild is entitled to delete that standalone `if`/`else` when bundling. It does. The module disappears, `build` and `transform` are never declared, and the first call throws `ReferenceError: build is not defined`. Only bundled builds tree shake, which is why `pnpm dev` was unaffected and the Docker image never worked. Initializing the bindings in their own declaration ties them to the used export, so tree shaking retains them. Two things hid the real error for the length of the investigation: - The `catch` branches in `build.ts` were inverted (documented as a known defect in AGENTS.md): a real `BuildFailure` was rethrown as `'Unknown Error'` while a non-`BuildFailure` was labelled `'Failed to Compile'` with the *Zod issue list* as its cause. Un-inverting them means `kind: 'ESBUILD_FAILURE'` now actually fires, so `CodeErrorBlock` can render. - `instrument-repos.service.ts` logged `String(err)`, discarding `.cause`. It now logs the structured cause. With tree shaking fixed, esbuild's JS *does* get inlined into `dist/app.js`, where esbuild refuses to run because it can no longer locate the native binary it shells out to. The installer stage now stages the binary belonging to the exact esbuild that was bundled, and `ESBUILD_BINARY_PATH` both points at it and suppresses that refusal. The version is resolved rather than pinned because the store holds several esbuild versions and a mismatched JS/binary pair refuses to start. `src/__tests__/vendor.test.ts` bundles the module with tree shaking enabled and asserts the dynamic import survives. It fails on the old source and passes on the new one. No existing unit or e2e test could have caught this: the e2e suite runs against the dev server, which is never bundled. Co-Authored-By: Claude Opus 5 --- apps/api/Dockerfile | 20 ++++++++++++- .../instrument-repos.service.ts | 6 +++- packages/instrument-bundler/AGENTS.md | 13 +++++---- .../src/__tests__/vendor.test.ts | 28 +++++++++++++++++++ packages/instrument-bundler/src/build.ts | 4 +-- .../instrument-bundler/src/vendor/esbuild.ts | 11 +++----- 6 files changed, 66 insertions(+), 16 deletions(-) create mode 100644 packages/instrument-bundler/src/__tests__/vendor.test.ts diff --git a/apps/api/Dockerfile b/apps/api/Dockerfile index 82143074b..ddcb6d37c 100644 --- a/apps/api/Dockerfile +++ b/apps/api/Dockerfile @@ -19,9 +19,21 @@ RUN turbo prune @opendatacapture/api FROM base AS installer COPY tsconfig.base.json vitest.config.ts ./ COPY --from=pruner /app/out/ . -RUN pnpm install --frozen-lockfile +RUN pnpm install --frozen-lockfile RUN turbo build --filter=@opendatacapture/api +# esbuild's JS API gets bundled into dist/app.js, from where it can no longer locate the native +# binary it shells out to. Stage the binary belonging to the exact esbuild that was bundled: the +# store holds several versions, and a mismatched JS/binary pair refuses to start. +RUN node -e "\ +const fs = require('node:fs'); \ +const path = require('node:path'); \ +const entry = require.resolve('esbuild', { paths: ['/app/packages/instrument-bundler'] }); \ +const scopeDir = path.join(path.dirname(entry), '../../@esbuild'); \ +const [platform] = fs.readdirSync(scopeDir); \ +fs.copyFileSync(path.join(scopeDir, platform, 'bin/esbuild'), '/esbuild-native'); \ +fs.chmodSync('/esbuild-native', 0o755);" + # RUN SERVER FROM base AS runner COPY --from=installer /app/apps/api/dist/ /app/dist/ @@ -30,5 +42,11 @@ COPY --from=installer /app/apps/api/dist/runtime/ /runtime/ RUN echo '{ "type": "module", "imports": { "#runtime/v1/*": "./dist/runtime/v1/*" } }' > package.json RUN echo '{ "type": "module" }' > /runtime/package.json + +# Setting this both points the bundled esbuild at its binary and suppresses its refusal to run +# from a bundle. Without it every instrument repo import fails to compile. +COPY --from=installer /esbuild-native /usr/local/bin/esbuild +ENV ESBUILD_BINARY_PATH="/usr/local/bin/esbuild" + USER node CMD [ "node", "--enable-source-maps", "./dist/app.js" ] diff --git a/apps/api/src/instrument-repos/instrument-repos.service.ts b/apps/api/src/instrument-repos/instrument-repos.service.ts index 3055e038c..ec3bfaff9 100644 --- a/apps/api/src/instrument-repos/instrument-repos.service.ts +++ b/apps/api/src/instrument-repos/instrument-repos.service.ts @@ -344,7 +344,11 @@ export class InstrumentReposService implements OnModuleInit { } } catch (err) { // One bad instrument should not abort importing the rest of the repository. - this.loggingService.error(`Failed to import instrument from ${path.basename(dir)}: ${String(err)}`); + this.loggingService.error({ + cause: err, + error: 'Failed to Import Instrument', + instrumentDir: path.basename(dir) + }); } } diff --git a/packages/instrument-bundler/AGENTS.md b/packages/instrument-bundler/AGENTS.md index 3a9c5f266..46d92bb93 100644 --- a/packages/instrument-bundler/AGENTS.md +++ b/packages/instrument-bundler/AGENTS.md @@ -45,15 +45,18 @@ degrades every such error, and **esbuild error locations are one line ahead of t matching `location.lineText` rather than trusting `location.line`. -**Known defect, verified, unfixed:** the two branches of the `catch` in `build.ts` are inverted. A real -`BuildFailure` parses successfully and is thrown as `'Unknown Error'` with no `kind`, so -`InstrumentBundlerError.isInstance(err, 'ESBUILD_FAILURE')` is never true and `CodeErrorBlock` never -renders. Anything you write that depends on `kind` will not fire until this is corrected. - **Never `import 'esbuild'` directly.** `src/vendor/esbuild.ts` switches between `esbuild` and `esbuild-wasm` on `typeof window === 'undefined'` — that is what lets the playground bundle in the browser. Tests also spy on this module (`vi.spyOn(esbuild, 'build')`). +**That switch must initialize its exports in the declaration itself.** `package.json` declares +`sideEffects: ['**/cli.ts']`, so every other file here is advertised as side-effect free and a +bundler may delete any standalone top-level statement. Writing the switch as an `if`/`else` that +assigns to a hoisted `var` puts the initialization in such a statement: `apps/api`'s production +bundle dropped the entire module and every instrument import failed with +`ReferenceError: build is not defined` — a failure that only appears in a bundled build, never under +`pnpm dev`. `src/__tests__/vendor.test.ts` bundles the module with tree shaking to guard this. + **`src/parse.ts` is vendored** from `parse-imports` (Apache-2.0, adapted to TypeScript and to run in a browser). Treat it as third-party: fix it upstream-style or not at all. diff --git a/packages/instrument-bundler/src/__tests__/vendor.test.ts b/packages/instrument-bundler/src/__tests__/vendor.test.ts new file mode 100644 index 000000000..a46460c16 --- /dev/null +++ b/packages/instrument-bundler/src/__tests__/vendor.test.ts @@ -0,0 +1,28 @@ +import * as path from 'node:path'; + +import { describe, expect, it } from 'vitest'; + +import * as esbuild from '../vendor/esbuild.js'; + +const VENDOR_DIR = path.resolve(import.meta.dirname, '../vendor'); + +describe('vendor/esbuild', () => { + it('should keep initializing its exports when tree shaken, so a bundled API does not lose the build binding', async () => { + const result = await esbuild.build({ + bundle: true, + external: ['esbuild', 'esbuild-wasm'], + format: 'esm', + keepNames: true, + platform: 'node', + stdin: { + contents: "import * as vendor from './esbuild.js'; export default vendor.build;", + loader: 'ts', + resolveDir: VENDOR_DIR + }, + target: ['node22', 'es2022'], + treeShaking: true, + write: false + }); + expect(result.outputFiles[0]!.text).toContain('await import("esbuild")'); + }); +}); diff --git a/packages/instrument-bundler/src/build.ts b/packages/instrument-bundler/src/build.ts index 5946967fe..dcb290fc1 100644 --- a/packages/instrument-bundler/src/build.ts +++ b/packages/instrument-bundler/src/build.ts @@ -67,9 +67,9 @@ export async function build({ } catch (err) { const parseResult = await $BuildFailure.safeParseAsync(err); if (parseResult.success) { - throw new InstrumentBundlerError('Unknown Error', { cause: err }); + throw new InstrumentBundlerError('Failed to Compile', { cause: parseResult.data, kind: 'ESBUILD_FAILURE' }); } - throw new InstrumentBundlerError('Failed to Compile', { cause: parseResult.error, kind: 'ESBUILD_FAILURE' }); + throw new InstrumentBundlerError('Unknown Error', { cause: err }); } return parseBuildResult(result); } diff --git a/packages/instrument-bundler/src/vendor/esbuild.ts b/packages/instrument-bundler/src/vendor/esbuild.ts index f9d65d5ef..2db39e3d0 100644 --- a/packages/instrument-bundler/src/vendor/esbuild.ts +++ b/packages/instrument-bundler/src/vendor/esbuild.ts @@ -1,5 +1,3 @@ -/* eslint-disable no-var */ - declare module 'esbuild' { // eslint-disable-next-line @typescript-eslint/no-unused-vars, @typescript-eslint/consistent-type-definitions export interface BuildResult { @@ -14,11 +12,10 @@ declare module 'esbuild-wasm' { } } -if (typeof window === 'undefined') { - var { build, transform } = await import('esbuild'); -} else { - var { build, transform } = await import('esbuild-wasm'); -} +// The bindings must be initialized by their own declaration, not by a detached `if`/`else`. +// This package declares `sideEffects: ['**/cli.ts']`, so a bundler is free to drop a +// standalone statement here, which silently leaves `build` undeclared in the output. +const { build, transform } = typeof window === 'undefined' ? await import('esbuild') : await import('esbuild-wasm'); export { build, transform }; export type { BuildFailure, BuildResult, Loader, Location, Message, Plugin } from 'esbuild'; From e3d9d4e94a960e5f6529e359b7abfc3945b210f8 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Tue, 4 Aug 2026 11:31:31 -0400 Subject: [PATCH 2/5] fix(api): use libnest nativeDependencies instead of manual Dockerfile staging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit libnest 8.4.0 generalizes the Prisma native-artifact pattern into a declarative `build.nativeDependencies` option. Adding `['esbuild']` to `libnest.config.ts` makes the build emit the binary beside `dist/app.js` and set `ESBUILD_BINARY_PATH` via the JS banner — replacing 14 lines of manual staging and env-var wiring in the Dockerfile. Co-Authored-By: Claude Opus 4.6 --- apps/api/Dockerfile | 17 ----------------- apps/api/libnest.config.ts | 1 + apps/api/package.json | 2 +- pnpm-lock.yaml | 10 +++++----- 4 files changed, 7 insertions(+), 23 deletions(-) diff --git a/apps/api/Dockerfile b/apps/api/Dockerfile index ddcb6d37c..51d216c70 100644 --- a/apps/api/Dockerfile +++ b/apps/api/Dockerfile @@ -22,18 +22,6 @@ COPY --from=pruner /app/out/ . RUN pnpm install --frozen-lockfile RUN turbo build --filter=@opendatacapture/api -# esbuild's JS API gets bundled into dist/app.js, from where it can no longer locate the native -# binary it shells out to. Stage the binary belonging to the exact esbuild that was bundled: the -# store holds several versions, and a mismatched JS/binary pair refuses to start. -RUN node -e "\ -const fs = require('node:fs'); \ -const path = require('node:path'); \ -const entry = require.resolve('esbuild', { paths: ['/app/packages/instrument-bundler'] }); \ -const scopeDir = path.join(path.dirname(entry), '../../@esbuild'); \ -const [platform] = fs.readdirSync(scopeDir); \ -fs.copyFileSync(path.join(scopeDir, platform, 'bin/esbuild'), '/esbuild-native'); \ -fs.chmodSync('/esbuild-native', 0o755);" - # RUN SERVER FROM base AS runner COPY --from=installer /app/apps/api/dist/ /app/dist/ @@ -43,10 +31,5 @@ COPY --from=installer /app/apps/api/dist/runtime/ /runtime/ RUN echo '{ "type": "module", "imports": { "#runtime/v1/*": "./dist/runtime/v1/*" } }' > package.json RUN echo '{ "type": "module" }' > /runtime/package.json -# Setting this both points the bundled esbuild at its binary and suppresses its refusal to run -# from a bundle. Without it every instrument repo import fails to compile. -COPY --from=installer /esbuild-native /usr/local/bin/esbuild -ENV ESBUILD_BINARY_PATH="/usr/local/bin/esbuild" - USER node CMD [ "node", "--enable-source-maps", "./dist/app.js" ] diff --git a/apps/api/libnest.config.ts b/apps/api/libnest.config.ts index c24daab19..8e607deda 100644 --- a/apps/api/libnest.config.ts +++ b/apps/api/libnest.config.ts @@ -26,6 +26,7 @@ declare module '@douglasneuroinformatics/libnest/user-config' { const config = defineUserConfig({ build: { + nativeDependencies: ['esbuild'], onComplete: async () => { const runtimeV1Dir = path.dirname( url.fileURLToPath(import.meta.resolve('@opendatacapture/runtime-v1/package.json')) diff --git a/apps/api/package.json b/apps/api/package.json index ed90a00fe..3dcccc129 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -21,7 +21,7 @@ "@casl/prisma": "^1.5.1", "@douglasneuroinformatics/libcrypto": "catalog:", "@douglasneuroinformatics/libjs": "catalog:", - "@douglasneuroinformatics/libnest": "^8.3.1", + "@douglasneuroinformatics/libnest": "^8.4.0", "@douglasneuroinformatics/libpasswd": "catalog:", "@douglasneuroinformatics/libstats": "catalog:", "@faker-js/faker": "^9.4.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c0e186b42..139732927 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -231,8 +231,8 @@ importers: specifier: 'catalog:' version: 3.2.1(neverthrow@8.2.0)(zod@vendor+zod@3.x) '@douglasneuroinformatics/libnest': - specifier: ^8.3.1 - version: 8.3.1(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24)(@nestjs/platform-fastify@11.1.24(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24))(@nestjs/testing@11.1.24(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24)(@nestjs/platform-express@11.1.24))(@prisma/client@6.19.3(prisma@6.19.3(typescript@6.0.3))(typescript@6.0.3))(@swc/types@0.1.26)(fastify@5.8.5)(neverthrow@8.2.0)(reflect-metadata@0.1.14)(rollup@4.61.1)(rxjs@7.8.2)(typescript@6.0.3)(vite@6.4.3(@types/node@24.13.0)(jiti@2.7.0)(lightningcss@1.32.0)(tsx@4.8.2)(yaml@2.9.0))(vitest@4.1.8)(zod@vendor+zod@3.x) + specifier: ^8.4.0 + version: 8.4.0(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24)(@nestjs/platform-fastify@11.1.24(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24))(@nestjs/testing@11.1.24(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24)(@nestjs/platform-express@11.1.24))(@prisma/client@6.19.3(prisma@6.19.3(typescript@6.0.3))(typescript@6.0.3))(@swc/types@0.1.26)(fastify@5.8.5)(neverthrow@8.2.0)(reflect-metadata@0.1.14)(rollup@4.61.1)(rxjs@7.8.2)(typescript@6.0.3)(vite@6.4.3(@types/node@24.13.0)(jiti@2.7.0)(lightningcss@1.32.0)(tsx@4.8.2)(yaml@2.9.0))(vitest@4.1.8)(zod@vendor+zod@3.x) '@douglasneuroinformatics/libpasswd': specifier: 'catalog:' version: 0.0.3(typescript@6.0.3) @@ -2145,8 +2145,8 @@ packages: neverthrow: ^8.2.0 zod: ^3.25.67 || 4.x - '@douglasneuroinformatics/libnest@8.3.1': - resolution: {integrity: sha512-YRO+0YNvru2dZzehGWYHbq/lZB/y39uwuSuW9GNmy9rW4d0lS+PiXSyM3EHYUU5uRIN3I+cB5zxUJTA9t+ZjQg==} + '@douglasneuroinformatics/libnest@8.4.0': + resolution: {integrity: sha512-qdX4vfRUvuQV2l0NoMov2ZKuoXUQhbi43rMqXgswWABzjoeiUcv4OjkM7tFShoHHvlS+5mfcMhkes0RUgbL22g==} engines: {node: 22.x || 24.x} hasBin: true peerDependencies: @@ -11900,7 +11900,7 @@ snapshots: type-fest: 4.41.0 zod: link:vendor/zod@3.x - '@douglasneuroinformatics/libnest@8.3.1(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24)(@nestjs/platform-fastify@11.1.24(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24))(@nestjs/testing@11.1.24(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24)(@nestjs/platform-express@11.1.24))(@prisma/client@6.19.3(prisma@6.19.3(typescript@6.0.3))(typescript@6.0.3))(@swc/types@0.1.26)(fastify@5.8.5)(neverthrow@8.2.0)(reflect-metadata@0.1.14)(rollup@4.61.1)(rxjs@7.8.2)(typescript@6.0.3)(vite@6.4.3(@types/node@24.13.0)(jiti@2.7.0)(lightningcss@1.32.0)(tsx@4.8.2)(yaml@2.9.0))(vitest@4.1.8)(zod@vendor+zod@3.x)': + '@douglasneuroinformatics/libnest@8.4.0(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24)(@nestjs/platform-fastify@11.1.24(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24))(@nestjs/testing@11.1.24(@nestjs/common@11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2))(@nestjs/core@11.1.24)(@nestjs/platform-express@11.1.24))(@prisma/client@6.19.3(prisma@6.19.3(typescript@6.0.3))(typescript@6.0.3))(@swc/types@0.1.26)(fastify@5.8.5)(neverthrow@8.2.0)(reflect-metadata@0.1.14)(rollup@4.61.1)(rxjs@7.8.2)(typescript@6.0.3)(vite@6.4.3(@types/node@24.13.0)(jiti@2.7.0)(lightningcss@1.32.0)(tsx@4.8.2)(yaml@2.9.0))(vitest@4.1.8)(zod@vendor+zod@3.x)': dependencies: '@douglasneuroinformatics/libjs': 3.2.1(neverthrow@8.2.0)(zod@vendor+zod@3.x) '@nestjs/common': 11.1.24(reflect-metadata@0.1.14)(rxjs@7.8.2) From cf8eb5f7344b88ff2bfed70dcfdb8b3d622fa364 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Tue, 4 Aug 2026 23:11:17 -0400 Subject: [PATCH 3/5] =?UTF-8?q?fix(instrument-bundler):=20address=20review?= =?UTF-8?q?=20=E2=80=94=20catch-branch=20regression=20test=20and=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add two tests to build.test.ts verifying the catch branches: a BuildFailure-shaped error becomes kind ESBUILD_FAILURE, a plain Error becomes an unknown error with no kind. - Remove the stale inverted-catch reference from odc-debugging/SKILL.md (the defect was fixed in aa9010d16). - Document nativeDependencies in apps/api/AGENTS.md Build section. Co-Authored-By: Claude Opus 4.6 --- .agents/skills/odc-debugging/SKILL.md | 9 +++--- apps/api/AGENTS.md | 5 ++++ .../src/__tests__/build.test.ts | 30 +++++++++++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/.agents/skills/odc-debugging/SKILL.md b/.agents/skills/odc-debugging/SKILL.md index 47ead450c..749501281 100644 --- a/.agents/skills/odc-debugging/SKILL.md +++ b/.agents/skills/odc-debugging/SKILL.md @@ -41,11 +41,10 @@ prerequisites rather than bugs: `.agents/skills/odc-run-locally/SKILL.md`. Turbo run outside turbo — a bare `tsc`, `vitest`, storybook — does not. **Some breakage is known, documented, and deliberately left alone.** Before calling anything a new bug, read the -`AGENTS.md` of the package that owns it — that is where these are recorded (the inverted `catch` in -`packages/instrument-bundler/src/build.ts`, the always-skipped development block in `packages/release-info`, -`testing`'s `test:chrome` naming a Playwright project that does not exist), with the build- and test-infrastructure -ones in the `Known warts` section of `.agents/docs/architecture/testing-strategy.md`. Name any you hit in your reply -and leave it as it is. +`AGENTS.md` of the package that owns it — that is where these are recorded (the always-skipped development block in +`packages/release-info`, `testing`'s `test:chrome` naming a Playwright project that does not exist), with the build- +and test-infrastructure ones in the `Known warts` section of `.agents/docs/architecture/testing-strategy.md`. Name any +you hit in your reply and leave it as it is. ## Quiet and wrong diff --git a/apps/api/AGENTS.md b/apps/api/AGENTS.md index c7cf5e98d..9ab3fb7fd 100644 --- a/apps/api/AGENTS.md +++ b/apps/api/AGENTS.md @@ -173,6 +173,11 @@ exist. `libnest.config.ts` copies `@opendatacapture/runtime-v1/dist` and the export worker into `dist/`. **Any other non-bundled runtime asset must be copied there too, or it will not exist in production.** +`nativeDependencies: ['esbuild']` in `libnest.config.ts` makes the build emit the esbuild native +binary to `dist/` and set `ESBUILD_BINARY_PATH` via the JS banner, so the Dockerfile needs no manual +binary staging. The binary is resolved from the application's dependency graph, not libnest's, so the +JS/binary pair always matches even when the two resolve different esbuild versions. + `#runtime/v1/*` is a Node subpath import declared in `apps/api/package.json` and mirrored in `apps/api/tsconfig.json` — two files that must agree. See `.agents/docs/architecture/runtime-and-vendor.md`. diff --git a/packages/instrument-bundler/src/__tests__/build.test.ts b/packages/instrument-bundler/src/__tests__/build.test.ts index 366327c3c..2c6acc6bc 100644 --- a/packages/instrument-bundler/src/__tests__/build.test.ts +++ b/packages/instrument-bundler/src/__tests__/build.test.ts @@ -72,6 +72,36 @@ describe('build', () => { await expect(build({ inputs })).rejects.toThrowError('expected at most one version of react'); }); + it('should wrap a BuildFailure-shaped error as ESBUILD_FAILURE', async () => { + vi.spyOn(esbuild, 'build').mockRejectedValueOnce({ + cause: undefined, + errors: [ + { + detail: undefined, + id: '', + location: null, + notes: [], + pluginName: '', + text: 'Could not resolve "missing"' + } + ], + message: 'Build failed with 1 error', + name: 'BuildFailure', + warnings: [] + }); + await expect(build(options)).rejects.toSatisfy((err: any) => { + return err.name === 'InstrumentBundlerError' && err.kind === 'ESBUILD_FAILURE'; + }); + }); + + it('should wrap a plain error as an unknown error with no kind', async () => { + const error = new TypeError('something broke'); + vi.spyOn(esbuild, 'build').mockRejectedValueOnce(error); + await expect(build(options)).rejects.toSatisfy((err: any) => { + return err.name === 'InstrumentBundlerError' && err.kind === undefined && err.cause === error; + }); + }); + // it('should return javascript that can be executed with no further transformation', () => { // const result = bundle(options); // expect((0, eval)(result.js)).toMatchObject({ kind: 'INTERACTIVE' }); From 9f5cdf77d31fc8fd90b74f63ed4007c0de21a884 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Tue, 4 Aug 2026 23:19:13 -0400 Subject: [PATCH 4/5] test(instrument-bundler): pin the catch-branch messages, not just the kind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review asked these tests to assert the thrown message as well as the kind. Both matter and they fail differently: `kind` gates whether `CodeErrorBlock` renders, while the message is what reaches the API's import log — which is what made the original inversion so hard to diagnose, since every real compile failure arrived labelled `Unknown Error` with a Zod issue list as its cause. Also asserts the `ESBUILD_FAILURE` cause carries the parsed `BuildFailure`, so a regression that passes `parseResult.error` (the Zod error) instead of `parseResult.data` is caught rather than silently discarding the diagnostics. Verified both tests fail when the catch branches are re-inverted. Co-Authored-By: Claude Opus 5 --- .../src/__tests__/build.test.ts | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/instrument-bundler/src/__tests__/build.test.ts b/packages/instrument-bundler/src/__tests__/build.test.ts index 2c6acc6bc..c2514c85d 100644 --- a/packages/instrument-bundler/src/__tests__/build.test.ts +++ b/packages/instrument-bundler/src/__tests__/build.test.ts @@ -72,8 +72,11 @@ describe('build', () => { await expect(build({ inputs })).rejects.toThrowError('expected at most one version of react'); }); - it('should wrap a BuildFailure-shaped error as ESBUILD_FAILURE', async () => { - vi.spyOn(esbuild, 'build').mockRejectedValueOnce({ + // These two branches shipped inverted, which relabelled every real compile failure and meant + // `CodeErrorBlock` never rendered. Both the message and the kind are asserted because each is + // load-bearing: the kind gates the error UI, and the message is what reaches the API log. + it('should report a BuildFailure as a compile failure, so the error UI can render it', async () => { + const buildFailure = { cause: undefined, errors: [ { @@ -88,17 +91,28 @@ describe('build', () => { message: 'Build failed with 1 error', name: 'BuildFailure', warnings: [] - }); + }; + vi.spyOn(esbuild, 'build').mockRejectedValueOnce(buildFailure); await expect(build(options)).rejects.toSatisfy((err: any) => { - return err.name === 'InstrumentBundlerError' && err.kind === 'ESBUILD_FAILURE'; + return ( + err.name === 'InstrumentBundlerError' && + err.message === 'Failed to Compile' && + err.kind === 'ESBUILD_FAILURE' && + err.cause.errors[0].text === 'Could not resolve "missing"' + ); }); }); - it('should wrap a plain error as an unknown error with no kind', async () => { + it('should report anything that is not a BuildFailure as an unknown error, preserving the cause', async () => { const error = new TypeError('something broke'); vi.spyOn(esbuild, 'build').mockRejectedValueOnce(error); await expect(build(options)).rejects.toSatisfy((err: any) => { - return err.name === 'InstrumentBundlerError' && err.kind === undefined && err.cause === error; + return ( + err.name === 'InstrumentBundlerError' && + err.message === 'Unknown Error' && + err.kind === undefined && + err.cause === error + ); }); }); From ae717238ff42a1aa2c59a0ff0d576694de10305a Mon Sep 17 00:00:00 2001 From: joshunrau Date: Wed, 5 Aug 2026 13:54:47 -0400 Subject: [PATCH 5/5] fix(instrument-bundler): keep the original error as the ESBUILD_FAILURE cause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parseResult.data` is the Zod-parsed copy, a plain object rather than the error esbuild threw, and two consumers gate on `instanceof Error`: `InstrumentErrorFallback` renders its `Cause` section only when `error.cause instanceof Error`, so that section stayed hidden for exactly the failures whose `kind` now makes `CodeErrorBlock` render, and libjs `formatError` walks the chain with the same check, so `serve-instrument` logged `Failed to Compile` with no cause beneath it. Passing `err` keeps both; the branch has already established that it parses, so the regression this guarded against — passing `parseResult.error` — remains excluded. The `BuildFailure` mock in the regression test was a plain object, which cannot tell the original apart from the parsed copy. It is now a real `Error` with the diagnostics attached, as esbuild throws, and the test asserts identity and `instanceof Error` alongside the existing `errors[0].text` check. Also name the underlying error in the other branch instead of `'Unknown Error'`: `Unexpected error while invoking esbuild: ReferenceError: build is not defined` is the string that would have identified the tree-shaking bug on sight, and it is what reaches any consumer that surfaces only `.message`. Both tests fail when the two lines in `build.ts` are reverted. Co-Authored-By: Claude Opus 5 (1M context) --- .../instrument-bundler/src/__tests__/build.test.ts | 13 +++++++++---- packages/instrument-bundler/src/build.ts | 13 ++++++++++--- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/packages/instrument-bundler/src/__tests__/build.test.ts b/packages/instrument-bundler/src/__tests__/build.test.ts index c2514c85d..d9d6c21d6 100644 --- a/packages/instrument-bundler/src/__tests__/build.test.ts +++ b/packages/instrument-bundler/src/__tests__/build.test.ts @@ -76,7 +76,10 @@ describe('build', () => { // `CodeErrorBlock` never rendered. Both the message and the kind are asserted because each is // load-bearing: the kind gates the error UI, and the message is what reaches the API log. it('should report a BuildFailure as a compile failure, so the error UI can render it', async () => { - const buildFailure = { + // esbuild throws a real Error with the diagnostics attached, and the cause must stay that + // error rather than the parsed copy: `InstrumentErrorFallback` gates its `Cause` section on + // `cause instanceof Error`, and libjs `formatError` walks the chain with the same check. + const buildFailure = Object.assign(new Error('Build failed with 1 error'), { cause: undefined, errors: [ { @@ -91,25 +94,27 @@ describe('build', () => { message: 'Build failed with 1 error', name: 'BuildFailure', warnings: [] - }; + }); vi.spyOn(esbuild, 'build').mockRejectedValueOnce(buildFailure); await expect(build(options)).rejects.toSatisfy((err: any) => { return ( err.name === 'InstrumentBundlerError' && err.message === 'Failed to Compile' && err.kind === 'ESBUILD_FAILURE' && + err.cause === buildFailure && + err.cause instanceof Error && err.cause.errors[0].text === 'Could not resolve "missing"' ); }); }); - it('should report anything that is not a BuildFailure as an unknown error, preserving the cause', async () => { + it('should name anything that is not a BuildFailure in the message, preserving the cause', async () => { const error = new TypeError('something broke'); vi.spyOn(esbuild, 'build').mockRejectedValueOnce(error); await expect(build(options)).rejects.toSatisfy((err: any) => { return ( err.name === 'InstrumentBundlerError' && - err.message === 'Unknown Error' && + err.message === 'Unexpected error while invoking esbuild: TypeError: something broke' && err.kind === undefined && err.cause === error ); diff --git a/packages/instrument-bundler/src/build.ts b/packages/instrument-bundler/src/build.ts index bf2352315..642bb6071 100644 --- a/packages/instrument-bundler/src/build.ts +++ b/packages/instrument-bundler/src/build.ts @@ -9,7 +9,7 @@ import * as esbuild from './vendor/esbuild.js'; import type { BundlerInput } from './schemas.js'; import type { BuildOutput } from './types.js'; -import type { BuildResult } from './vendor/esbuild.js'; +import type { BuildFailure, BuildResult } from './vendor/esbuild.js'; const DEFAULT_REACT_PACKAGE = 'react@19.x'; @@ -37,6 +37,10 @@ function resolveJsxImportSource(inputs: BundlerInput[]): string { return `/runtime/v1/${packages.values().next().value ?? DEFAULT_REACT_PACKAGE}`; } +function describeError(err: unknown): string { + return err instanceof Error ? `${err.name}: ${err.message}` : `${typeof err}: ${String(err)}`; +} + function parseBuildResult(result: BuildResult): BuildOutput { const cssOutput = result.outputFiles?.find((output) => output.path.endsWith('bundle.css')); const jsOutput = result.outputFiles?.find((output) => output.path.endsWith('bundle.js')); @@ -95,9 +99,12 @@ export async function build({ } catch (err) { const parseResult = await $BuildFailure.safeParseAsync(err); if (parseResult.success) { - throw new InstrumentBundlerError('Failed to Compile', { cause: parseResult.data, kind: 'ESBUILD_FAILURE' }); + // the original error, rather than the parsed copy, so that `cause instanceof Error` holds downstream + throw new InstrumentBundlerError('Failed to Compile', { cause: err as BuildFailure, kind: 'ESBUILD_FAILURE' }); } - throw new InstrumentBundlerError('Unknown Error', { cause: err }); + // anything esbuild did not report as a compilation failure is a fault in the bundler itself, not in the + // instrument, so name it here rather than leaving the reader with 'Unknown Error' and a stack + throw new InstrumentBundlerError(`Unexpected error while invoking esbuild: ${describeError(err)}`, { cause: err }); } return parseBuildResult(result); }