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 c3d97da8a..8e69d7aae 100644 --- a/apps/api/AGENTS.md +++ b/apps/api/AGENTS.md @@ -178,6 +178,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/apps/api/Dockerfile b/apps/api/Dockerfile index 82143074b..51d216c70 100644 --- a/apps/api/Dockerfile +++ b/apps/api/Dockerfile @@ -19,7 +19,7 @@ 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 # RUN SERVER @@ -30,5 +30,6 @@ 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 + 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/apps/api/src/instrument-repos/instrument-repos.service.ts b/apps/api/src/instrument-repos/instrument-repos.service.ts index e49e51fe6..b74d7a18c 100644 --- a/apps/api/src/instrument-repos/instrument-repos.service.ts +++ b/apps/api/src/instrument-repos/instrument-repos.service.ts @@ -329,7 +329,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 8246a273d..e093123b2 100644 --- a/packages/instrument-bundler/AGENTS.md +++ b/packages/instrument-bundler/AGENTS.md @@ -55,15 +55,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__/build.test.ts b/packages/instrument-bundler/src/__tests__/build.test.ts index 366327c3c..d9d6c21d6 100644 --- a/packages/instrument-bundler/src/__tests__/build.test.ts +++ b/packages/instrument-bundler/src/__tests__/build.test.ts @@ -72,6 +72,55 @@ describe('build', () => { await expect(build({ inputs })).rejects.toThrowError('expected at most one version of react'); }); + // 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 () => { + // 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: [ + { + detail: undefined, + id: '', + location: null, + notes: [], + pluginName: '', + text: 'Could not resolve "missing"' + } + ], + 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 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 === 'Unexpected error while invoking esbuild: TypeError: something broke' && + 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' }); 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 53aeb1a87..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('Unknown Error', { cause: err }); + // 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('Failed to Compile', { cause: parseResult.error, kind: 'ESBUILD_FAILURE' }); + // 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); } 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'; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 515d915bf..b5585e37a 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) @@ -2151,8 +2151,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: @@ -11903,7 +11903,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)