fix(instrument-bundler): keep the esbuild switch alive through tree shaking

[thomasbeaudry]

The bug

Adding a GitHub instrument repository imports 0 instruments on any production
deployment, while the identical repo imports correctly under pnpm dev.

Every instrument fails, and the log gives nothing to work with:

Found 24 instrument directories in DouglasNeuroInformatics/ODC_Instruments
Failed to import instrument from ADHD_ASRS_1.1: InstrumentBundlerError: Failed to Compile
... (x24)
Imported 0 instruments from DouglasNeuroInformatics/ODC_Instruments

This has never worked in a container. The feature is only usable in development.

Root cause

packages/instrument-bundler/src/vendor/esbuild.ts picked between the native and
WASM builds of esbuild with a detached statement:

if (typeof window === 'undefined') {
  var { build, transform } = await import('esbuild');
} else {
  var { build, transform } = await import('esbuild-wasm');
}

export { build, transform };

This relies on var hoisting: the declarations float to module scope, and a
standalone if/else assigns them.

packages/instrument-bundler/package.json declares:

"sideEffects": ["**/cli.ts"]

which advertises every other file in the package as side-effect free. A
bundler is therefore entitled to delete any standalone top-level statement in
this module — and esbuild does exactly that when apps/api is bundled for
production. The whole module vanishes from dist/app.js. Nothing declares
build or transform, so the first call throws:

ReferenceError: build is not defined
    at build (/packages/instrument-bundler/src/build.ts:32:9)
    at bundle (/packages/instrument-bundler/src/bundle.ts:65:26)
    at InstrumentReposService.importInstrumentFromDir (...)

Only bundled builds tree shake. pnpm dev loads the module natively and works,
which is why this never showed up in development or in CI.

Reproduction

Bundling a module with this shape, inside a package declaring sideEffects,
with the same options libnest uses (--bundle --format=esm --platform=node --keep-names --target=node22,es2022), produces output where the module is gone
entirely and the call site references an undeclared identifier. Running it
reproduces ReferenceError: build is not defined verbatim.

The fix

Initialize the bindings in their own declaration, so they are reachable from the
used export and tree shaking must retain them:

const { build, transform } =
  typeof window === 'undefined' ? await import('esbuild') : await import('esbuild-wasm');

Runtime semantics are unchanged — both dynamic imports are still separate, still
code-split, and still only one branch executes — so the playground's browser path
is unaffected.

Second failure behind the first

With tree shaking fixed, esbuild's JS is now inlined into dist/app.js, and
esbuild ships a guard against precisely that:

if ((!ESBUILD_BINARY_PATH || false) &&
    (path.basename(__filename) !== "main.js" || path.basename(__dirname) !== "lib")) {
  throw new Error('The esbuild JavaScript API cannot be bundled. ...');
}

It refuses to run from a bundle because it can no longer find the native binary
it shells out to. libnest exposes no way to mark a package external
(build.bundle: false externalizes all of node_modules, which would change
the deployment model), so apps/api/Dockerfile now:

• stages the native binary belonging to the exact esbuild that was bundled.
  The version is resolved through require.resolve rather than pinned, because
  the pnpm store holds four esbuild versions and a mismatched JS/binary pair
  refuses to start. The platform directory is read rather than hardcoded, so this
  is not arch-specific.
• sets ESBUILD_BINARY_PATH, which both points esbuild at that binary and
  suppresses the guard above.

Verified both ways: without the variable the bundle throws cannot be bundled;
with it, the build succeeds.

Why the error was invisible

Two layers discarded the cause, which is why the symptom was an unactionable
Failed to Compile rather than a ReferenceError:

1. The catch branches in build.ts were inverted. This was already
   recorded in AGENTS.md as a known, verified, unfixed defect. A real
   BuildFailure parsed successfully and was rethrown as 'Unknown Error',
   while anything that wasn't a BuildFailure was labelled 'Failed to Compile' with parseResult.error — the Zod issue list — as its cause.
   So the one thing the log could have shown was a complaint that the thrown
   value lacked errors and warnings arrays. Un-inverting them also means
   kind: 'ESBUILD_FAILURE' fires for the first time, so
   InstrumentErrorFallback's CodeErrorBlock can finally render.

2. instrument-repos.service.ts logged String(err), which stringifies to
   the message and drops .cause entirely. It now logs the structured cause,
   matching the { cause, error } shape used in gateway.synchronizer.ts.

Tests

packages/instrument-bundler/src/__tests__/vendor.test.ts bundles the vendor
module with tree shaking enabled and asserts the dynamic import survives.
Confirmed to fail on the old source and pass on the new one.

No existing test could have caught this. Unit tests import the module directly
and never bundle it, and testing/src/specs/admin-instrument-repos.spec.ts runs
against the dev server, which is never tree shaken. A bundling regression is
only observable in a bundled artifact, so the guard has to live at that layer.

Full suite: 413 passed, 1 skipped. tsc and eslint clean.

🤖 Generated with Claude Code

[thomasbeaudry]

Follow-up: the Dockerfile workaround here should not be permanent

The ESBUILD_BINARY_PATH staging in apps/api/Dockerfile works, but it is a per-consumer
workaround for a gap in libnest: buildProd has no way to declare a package that cannot
survive bundling, and build.bundle: false is all-or-nothing.

DouglasNeuroInformatics/libnest#76 adds a generic mechanism —
build: { nativeDependencies: ['esbuild'] } — by generalizing the existing prismaPlugin
pattern (locate the artifact, redirect via the banner, emit it beside the bundle).

This PR is deliberately not blocked on that, for two reasons:

1. It fixes a production breakage — instrument repo import fails on every containerized
   deployment today. Coupling it to an unmerged upstream PR delays the fix for a review
   timeline outside this repo's control.

2. Adopting the option early would fail silently. $UserConfigOptions in libnest's
   load.ts is a plain z.object, which strips unknown keys rather than rejecting them —
   verified against the version currently resolved here:

   parsed ok: true
   surviving keys: [ 'outfile' ]
   

   So declaring nativeDependencies against libnest 8.3.1 validates cleanly, is discarded,
   the plugin never runs, and the build succeeds producing a bundle with no banner and no
   staged binary. lint-and-test does not build the image or exercise instrument import, so
   CI would be green and the published image would be broken in exactly the way this PR
   exists to fix.

Note that minimumReleaseAge is not a blocker — @douglasneuroinformatics/* is listed
in minimumReleaseAgeExclude, so a new libnest can be adopted as soon as it publishes.

Once libnest#76 merges and ships, a small follow-up here deletes 14 lines from the
Dockerfile (the RUN node -e staging block, the COPY --from=installer /esbuild-native, and
the ENV ESBUILD_BINARY_PATH) and adds one line to libnest.config.ts. The binary then
rides the existing COPY --from=installer /app/apps/api/dist/, so no replacement copy step
is needed.

As a bonus, libnest#76 fails the build when a declared native dependency never enters the
graph — which would have caught the tree-shaking regression this PR fixes at build time,
rather than in production. No test in this repo can currently catch it, since the e2e suite
runs against the never-bundled dev server.

[gdevenyi]

Solid fix, and the investigation writeup made it straightforward to verify. I reproduced the guard test failing on the old source, confirmed a local production build emits the version-matched esbuild binary with the banner in place, and then verified the whole mechanism end to end in a real container: built the image from this PR's merge result, confirmed dist/esbuild (0.23.1, executable) and the ESBUILD_BINARY_PATH banner inside it, booted the api against a mongo replica set, and ran setup — the default instrument repo import fetched and bundled 24/24 instruments from DouglasNeuroInformatics/ODC_Instruments with zero per-instrument failures. That is the exact path that imported 0 before this change.

(One caveat for the record, not caused by this PR: freshly built images of current main crash at boot from an unrelated globalThis.__dirname collision between libnest's bundle banner and Prisma's pre-bundled engines chunk — a control build at the merge base reproduces it identically. The smoke test worked around it; it is being reported separately.)

Three small things before this lands:

1. Add a regression test for the catch you un-inverted in packages/instrument-bundler/src/build.ts. In src/__tests__/build.test.ts, mock esbuild.build to reject once with a $BuildFailure-shaped object and assert the thrown error has kind: 'ESBUILD_FAILURE' and message 'Failed to Compile', and once with a plain Error, asserting 'Unknown Error'. These branches shipped inverted once already; vendor.test.ts guards the tree-shaking half, but nothing pins this one.

2. .agents/skills/odc-debugging/SKILL.md (lines 44-45) still lists the inverted catch in build.ts as a known, deliberately-unfixed defect. You fixed it and updated the bundler AGENTS.md, but this reference is now stale — please drop it from that example list.

3. apps/api/AGENTS.md, "Build" section: it says non-bundled runtime assets must be copied via build.onComplete or they will not exist in production. The esbuild binary is now such an asset handled by a different mechanism — add a sentence that nativeDependencies: ['esbuild'] makes libnest emit the version-matched binary into dist/ and point ESBUILD_BINARY_PATH at it via the bundle banner.

If you hand this to Claude Code, Fable 5 is the right size — the work touches the instrument pipeline.

Reviewed at commit e3d9d4e.

[thomasbeaudry]

Thanks for the container-level verification — 24/24 on the exact path that imported 0 is the confirmation that mattered.

All three items are addressed. Two were already in cf8eb5f7; I've since tightened the first.

1. Catch-branch regression test — src/__tests__/build.test.ts now covers both branches. The initial version asserted name/kind/cause but not the message you asked for, so I've added it, along with an assertion that the ESBUILD_FAILURE cause carries the parsed BuildFailure:

err.name === 'InstrumentBundlerError' &&
err.message === 'Failed to Compile' &&
err.kind === 'ESBUILD_FAILURE' &&
err.cause.errors[0].text === 'Could not resolve "missing"'

That last clause matters independently: the original code passed parseResult.error — the Zod issue list — as the cause, so a regression reintroducing it would throw away the actual esbuild diagnostics while still looking correct on kind. Both tests were confirmed to fail with the branches re-inverted.

2. odc-debugging/SKILL.md — the inverted-catch entry is dropped from the known-defects list.

3. apps/api/AGENTS.md — the Build section now documents nativeDependencies: ['esbuild'], including that the binary is resolved from the application's dependency graph rather than libnest's, so the JS/binary pair matches even when the two resolve different esbuild versions.

main is merged in (6a894fc5) and the branch has nothing behind it. Full suite green at 616 passed, 1 skipped; tsc and eslint clean for instrument-bundler.

On the globalThis.__dirname collision you flagged: agreed it is unrelated and pre-existing — it reproduces at the merge base. Worth noting for whoever picks it up that it is the same class of problem this PR's upstream fix addresses, since libnest's banner is where __dirname is defined. DouglasNeuroInformatics/libnest#76 (merged, shipped in 8.4.0) is what replaced the manual Dockerfile staging here.

[joshunrau]

I hit this same bug from the other end — serve-instrument@2.2.0 cannot compile any instrument locally, because its published dist/cli.js contains no reference to esbuild at all. Same sideEffects → dropped vendor module → ReferenceError: build is not defined, and the same inverted catch turning it into a ZodError about missing errors/warnings arrays. I'd opened #1493 before finding this PR and have closed it — your fix is the better one, since initializing the bindings from their own declaration removes the dependency on the sideEffects list rather than adding to it, and vendor.test.ts guards it at the layer where it actually breaks.

Two small things from my version that may be worth folding in here:

1. cause: parseResult.data drops instanceof Error downstream. $BuildFailure is a z.object, so parseResult.data is a plain object rather than the original error. Two consumers gate on instanceof Error:

• packages/react-core/src/components/InstrumentErrorFallback/InstrumentErrorFallback.tsx:41 gates the Cause toggle on error.cause instanceof Error, so that section silently never renders for esbuild failures — the same branch whose kind you just fixed to make CodeErrorBlock render.
• libjs formatError walks the chain with while (cause instanceof Error), so serve-instrument's logError prints only InstrumentBundlerError: Failed to Compile with no cause. (esbuild's own stderr output at logLevel: 'warning' still shows the diagnostic there, so this one is mostly cosmetic in the CLI.)

Passing the original error keeps both, and still satisfies the guard you added — build.test.ts asserts err.cause.errors[0].text, which is shape-based, so identity with the parsed copy is not required:

throw new InstrumentBundlerError('Failed to Compile', { cause: err as BuildFailure, kind: 'ESBUILD_FAILURE' });

The regression you were guarding against (passing parseResult.error) is equally excluded, since the branch has already established that err parses.

2. Naming the underlying error in the non-BuildFailure branch. 'Unknown Error' is accurate but not actionable where only .message is surfaced:

throw new InstrumentBundlerError(`Unexpected error while invoking esbuild: ${describeError(err)}`, { cause: err });
// -> "Unexpected error while invoking esbuild: ReferenceError: build is not defined"

That is the string that would have identified this bug on sight. Minor given you have already fixed instrument-repos.service.ts to log the structured cause — take it or leave it.

Neither is blocking; the fix as it stands resolves my case.

[joshunrau]

Pushed both suggestions to this branch in ae71723, since it was quicker than describing them.

• ESBUILD_FAILURE now carries err rather than parseResult.data, so cause instanceof Error holds for InstrumentErrorFallback's Cause section and for libjs formatError.
• The other branch names the error: Unexpected error while invoking esbuild: ReferenceError: build is not defined.

Your regression test needed a change to go with it: the BuildFailure mock was a plain object, which cannot tell the original error apart from the parsed copy, so it passed either way. It is now a real Error with the diagnostics attached — which is what esbuild actually throws — and asserts identity and instanceof Error alongside the existing errors[0].text check. The second test asserts the new message. Both fail when the two lines in build.ts are reverted.

tsc and eslint clean, 45 tests pass. Revert the commit if you would rather keep the parsed copy — the fix stands either way.