From 1af7fd66ba671771713175eebb33d27ac9db94dc Mon Sep 17 00:00:00 2001 From: Thibaut Fatus Date: Mon, 31 Aug 2026 10:41:07 +0200 Subject: [PATCH] [fix] resolve the pack scope per environment, not through node:async_hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit packs.ts created its AsyncLocalStorage from a static `node:async_hooks` import. The browser reaches that module — Mechanism.listAll() and RiskCategory.listAll() both read Packs.current() — so every client bundle importing the barrel pulled a node builtin in. Bundlers externalize the builtin to a stub and then fail on the named import; kora-infra's website build has been broken since #27: "AsyncLocalStorage" is not exported by "__vite-browser-external", imported by ".../packs/packs.js" The scope now comes in through the `#packScope` subpath import, resolved by condition in package.json: - node/workers (default): packScope.node.ts, the real AsyncLocalStorage. A server runs several runs concurrently in one isolate, so the active pack still has to follow each one across its awaits. - browser: packScope.browser.ts, a save/restore stack with no node import. A page renders against one pack, and `Packs.run()` is only ever called with a synchronous callback, so the stack is accurate for that shape. Both conditions point at build output, so the specifier resolves for consumers but not for vitest running from source; the vitest config aliases it back to the node implementation, which also keeps an unbuilt checkout testable. Tests cover both implementations against the same behavior — no store outside run(), the store visible inside, and the enclosing store restored on return and on throw — plus a guard that packs.ts imports no node builtin, since nothing else would catch the regression before a downstream client build. tsbuild, 237 tests and prettier pass; lint unchanged (1 pre-existing warning). --- packages/benchmark/package.json | 7 +++ .../src/packs/__tests__/packScope.test.ts | 52 +++++++++++++++++++ .../benchmark/src/packs/packScope.browser.ts | 32 ++++++++++++ .../benchmark/src/packs/packScope.node.ts | 11 ++++ packages/benchmark/src/packs/packScope.ts | 16 ++++++ packages/benchmark/src/packs/packs.ts | 8 +-- vitest.config.ts | 17 ++++++ 7 files changed, 140 insertions(+), 3 deletions(-) create mode 100644 packages/benchmark/src/packs/__tests__/packScope.test.ts create mode 100644 packages/benchmark/src/packs/packScope.browser.ts create mode 100644 packages/benchmark/src/packs/packScope.node.ts create mode 100644 packages/benchmark/src/packs/packScope.ts diff --git a/packages/benchmark/package.json b/packages/benchmark/package.json index a1c2805..3d60dce 100644 --- a/packages/benchmark/package.json +++ b/packages/benchmark/package.json @@ -36,6 +36,13 @@ "import": "./build/src/index.js" } }, + "imports": { + "#packScope": { + "types": "./src/packs/packScope.node.ts", + "browser": "./build/src/packs/packScope.browser.js", + "default": "./build/src/packs/packScope.node.js" + } + }, "dependencies": { "@korabench/core": "^1.0.6", "remeda": "^2.33.6", diff --git a/packages/benchmark/src/packs/__tests__/packScope.test.ts b/packages/benchmark/src/packs/__tests__/packScope.test.ts new file mode 100644 index 0000000..29644df --- /dev/null +++ b/packages/benchmark/src/packs/__tests__/packScope.test.ts @@ -0,0 +1,52 @@ +import {readFileSync} from "node:fs"; +import {describe, expect, it} from "vitest"; +import {createPackScope as createBrowserScope} from "../packScope.browser.js"; +import {createPackScope as createNodeScope} from "../packScope.node.js"; + +describe.each([ + ["browser", createBrowserScope], + ["node", createNodeScope], +])("%s pack scope", (_name, create) => { + it("has no store outside run()", () => { + expect(create().getStore()).toBeUndefined(); + }); + + it("exposes the store to the callback and returns its value", () => { + const scope = create(); + + expect(scope.run("alpha", () => scope.getStore())).toBe("alpha"); + }); + + it("restores the enclosing store, including when fn throws", () => { + const scope = create(); + + scope.run("outer", () => { + scope.run("inner", () => undefined); + expect(scope.getStore()).toBe("outer"); + + expect(() => + scope.run("inner", () => { + throw new Error("boom"); + }) + ).toThrow("boom"); + expect(scope.getStore()).toBe("outer"); + }); + + expect(scope.getStore()).toBeUndefined(); + }); +}); + +// packs.ts is reached from the browser through Mechanism and RiskCategory, so a +// static node builtin in it breaks every client bundle that touches the barrel: +// the build externalizes the builtin to a stub and fails on the named import. +// The scope has to keep coming in through `#packScope`. +describe("packs.ts", () => { + it("imports no node builtin", () => { + const source = readFileSync( + new URL("../packs.ts", import.meta.url), + "utf8" + ); + + expect(source).not.toMatch(/from\s+"node:/); + }); +}); diff --git a/packages/benchmark/src/packs/packScope.browser.ts b/packages/benchmark/src/packs/packScope.browser.ts new file mode 100644 index 0000000..192bd29 --- /dev/null +++ b/packages/benchmark/src/packs/packScope.browser.ts @@ -0,0 +1,32 @@ +import type {PackScope} from "./packScope.js"; + +/** + * The browser stand-in, selected by the `browser` condition on `#packScope`. + * + * A page renders against one pack, so there is no concurrent work to keep + * apart and a save/restore stack is enough. It is accurate for the only shape + * `Packs.run()` is called with — a synchronous callback — and, unlike + * `AsyncLocalStorage`, does not survive an await; a browser caller that needs + * that would have to reach for the real thing. + */ +class StackScope implements PackScope { + #store: T | undefined; + + getStore(): T | undefined { + return this.#store; + } + + run(store: T, fn: () => R): R { + const previous = this.#store; + this.#store = store; + try { + return fn(); + } finally { + this.#store = previous; + } + } +} + +export function createPackScope(): PackScope { + return new StackScope(); +} diff --git a/packages/benchmark/src/packs/packScope.node.ts b/packages/benchmark/src/packs/packScope.node.ts new file mode 100644 index 0000000..bc3fc62 --- /dev/null +++ b/packages/benchmark/src/packs/packScope.node.ts @@ -0,0 +1,11 @@ +import {AsyncLocalStorage} from "node:async_hooks"; +import type {PackScope} from "./packScope.js"; + +/** + * The real async-context scope, used by the CLI, Node scripts and the worker. + * A server can have several runs in flight in one isolate, so the active pack + * has to follow each one across its awaits. + */ +export function createPackScope(): PackScope { + return new AsyncLocalStorage(); +} diff --git a/packages/benchmark/src/packs/packScope.ts b/packages/benchmark/src/packs/packScope.ts new file mode 100644 index 0000000..f8d9ae1 --- /dev/null +++ b/packages/benchmark/src/packs/packScope.ts @@ -0,0 +1,16 @@ +/** + * The slice of `AsyncLocalStorage` the active-pack scope needs. + * + * `packs.ts` reaches an implementation through the `#packScope` subpath + * import, which resolves to `packScope.node.ts` everywhere except a browser + * bundle (see the `imports` map in package.json). Node's `AsyncLocalStorage` + * lives behind `node:async_hooks`, and a static import of that anywhere the + * browser can reach breaks bundlers: the client build externalizes the builtin + * to a stub and then fails on the named import. Every browser consumer of this + * package reaches `Packs.current()` through `Mechanism` and `RiskCategory`, so + * that path has to stay free of node builtins. + */ +export interface PackScope { + getStore(): T | undefined; + run(store: T, fn: () => R): R; +} diff --git a/packages/benchmark/src/packs/packs.ts b/packages/benchmark/src/packs/packs.ts index 2e2db20..1dffa27 100644 --- a/packages/benchmark/src/packs/packs.ts +++ b/packages/benchmark/src/packs/packs.ts @@ -1,4 +1,4 @@ -import {AsyncLocalStorage} from "node:async_hooks"; +import {createPackScope} from "#packScope"; import {BehaviorSet} from "./behaviorSet.js"; import {bundledPacks} from "./bundled.js"; import {PackStamp} from "./packStamp.js"; @@ -28,13 +28,15 @@ export interface PacksOverride { // --behaviors once per invocation and every command reads it. // - `storage` is an async-context scope. kora-infra serves several runs // concurrently from a single Cloudflare isolate, so a mutable global is not -// enough there; each run wraps its work in `Packs.run(...)`. +// enough there; each run wraps its work in `Packs.run(...)`. It comes from +// `#packScope` rather than `node:async_hooks` directly, so that a browser +// bundle can resolve a node-free implementation — see packScope.ts. // // Callers should pick one. Mixing them is legal but makes it much harder to // reason about which pack a given schema was built from. // -const storage = new AsyncLocalStorage(); +const storage = createPackScope(); let configured: ActivePacks | undefined; diff --git a/vitest.config.ts b/vitest.config.ts index 2b1540d..93983ab 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,6 +1,23 @@ +import path from "node:path"; +import {fileURLToPath} from "node:url"; import {defineConfig} from "vitest/config"; +const thisDir = path.dirname(fileURLToPath(import.meta.url)); + export default defineConfig({ + resolve: { + // `#packScope` resolves through package.json conditions, which point at + // build output so that Node gets the AsyncLocalStorage scope and a browser + // bundle gets the node-free one. Tests run from source, and an unbuilt + // checkout should still be testable, so point it back at the source of the + // implementation Node would have picked. + alias: { + "#packScope": path.resolve( + thisDir, + "packages/benchmark/src/packs/packScope.node.ts" + ), + }, + }, test: { include: ["packages/*/src/**/*.test.ts"], },