diff --git a/AGENTS.md b/AGENTS.md index 84cbf45e1..748cc8b21 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,7 +29,16 @@ This repo contains multiple packages, but **`packages/coding-agent/`** is the pr - **Class privacy**: use ES `#private` fields; leave externally accessible members bare. **No `private`/`protected`/`public` keyword on fields or methods**, except on **constructor parameter properties** where TypeScript requires it (e.g. `constructor(private readonly session: ToolSession)`). - **Promises**: use `Promise.withResolvers()` instead of `new Promise((resolve, reject) => ...)`. - **Prompts**: never build prompts in code (no inline strings, template literals, or concatenation). Prompts live in static `.md` files; use Handlebars for dynamic content. Import them via `import content from "./prompt.md" with { type: "text" }` — not `readFile`. -- **Worker scripts**: reference the entry via `import workerUrl from "./worker.ts" with { type: "file" }` so the path survives bundling. tsgo flags it (TS1192/TS5097) — suppress with `// @ts-expect-error -- Bun file-URL import` directly above the line (see `tab-supervisor.ts`, `context-manager.ts`, `aggregator.ts`). +- **Worker scripts**: spawn workers with the dev/compile-safe hybrid pattern. `with { type: "file" }` only copies the entry as a raw asset and does **not** bundle its imports — workers crashed silently in compiled binaries on every prior incarnation of that pattern (issues #1011, #1027). Use this shape instead: + ```ts + import { isCompiledBinary } from "@oh-my-pi/pi-utils"; + const worker = isCompiledBinary() + ? new Worker("./packages//src/.ts", { type: "module" }) + : new Worker(new URL("./.ts", import.meta.url).href, { type: "module" }); + ``` + The literal in the compiled branch is what Bun's `--compile` static analyzer needs to discover the worker — its path is **`--root`-relative** (repo root, since `build-binary.ts` passes `--root ../..`), so it must start with `./packages/...`. The `new URL` form in the dev branch keeps spawns portable across cwds. + In addition, every worker entry **MUST** be listed as an extra `--compile` entrypoint in `packages/coding-agent/scripts/build-binary.ts`. Without that the analyzer sees the literal but the worker never gets emitted into bunfs. The three current entries (`sync-worker.ts`, `tab-worker-entry.ts`, `worker-entry.ts`) live there as the working reference. + Validate any new worker with the dedicated smoke probe: `omp --smoke-test` spawns the stats sync worker, pings it, and exits — it's wired into `ci:test:smoke` and `scripts/install-tests/run-ci.sh` so binary, source-link, and tarball installs all exercise it. Add a sibling smoke if the new worker is on a different module graph. ## Bun Over Node diff --git a/package.json b/package.json index 0e5bb84e9..333e71a31 100644 --- a/package.json +++ b/package.json @@ -109,7 +109,7 @@ "ci:check:full": "bun run check:ts", "ci:build:native": "bun scripts/ci-build-native.ts", "ci:test:full": "bun run test", - "ci:test:smoke": "bun packages/coding-agent/src/cli.ts --version && bun packages/coding-agent/src/cli.ts --help && bun packages/coding-agent/src/cli.ts stats --help", + "ci:test:smoke": "bun packages/coding-agent/src/cli.ts --version && bun packages/coding-agent/src/cli.ts --help && bun packages/coding-agent/src/cli.ts stats --help && bun packages/coding-agent/src/cli.ts --smoke-test", "ci:test:install-methods": "bash scripts/install-tests/run-ci.sh", "ci:release:build-binaries": "bun scripts/ci-release-build-binaries.ts", "ci:release:publish": "bun scripts/ci-release-publish.ts", diff --git a/packages/coding-agent/scripts/build-binary.ts b/packages/coding-agent/scripts/build-binary.ts index fa5de7caf..0c56afa56 100644 --- a/packages/coding-agent/scripts/build-binary.ts +++ b/packages/coding-agent/scripts/build-binary.ts @@ -40,6 +40,17 @@ async function main(): Promise { "--root", "../..", "./src/cli.ts", + // Worker entrypoints. Bun's `--compile` discovers the literal in + // `new Worker("…", …)` at each spawn site, but only actually + // emits the worker into the bunfs root when it is listed here as + // an explicit additional entry. Paths are relative to this + // script's cwd (packages/coding-agent) and the `--root` above + // (../..) makes them appear inside the binary at + // `/$bunfs/root/packages//src/.js`, which is + // exactly what the literals at the spawn sites resolve to. + "../stats/src/sync-worker.ts", + "./src/tools/browser/tab-worker-entry.ts", + "./src/eval/js/worker-entry.ts", "--outfile", "dist/omp", ], diff --git a/packages/coding-agent/src/cli.ts b/packages/coding-agent/src/cli.ts index d59454e63..cafc4692e 100755 --- a/packages/coding-agent/src/cli.ts +++ b/packages/coding-agent/src/cli.ts @@ -84,8 +84,30 @@ function isSubcommand(first: string | undefined): boolean { return commands.some(e => e.name === first || e.aliases?.includes(first)); } +/** + * Smoke-test entry. Spawns the stats sync worker, pings it, exits. + * + * Purpose: catch the silent worker-load regressions that hit compiled + * binaries (issues #1011 and #1027). Neither `--version` nor + * `stats --summary` actually spawns a Worker on a fresh install — the + * sync path early-returns when no session files exist. This probe is the + * minimal end-to-end test that proves `new Worker(...)` resolves and the + * bundled worker module evaluates successfully. Wired into + * `scripts/install-tests/run-ci.sh` so binary / source-link / tarball + * installs all exercise it on every CI run. + */ +async function runSmokeTest(): Promise { + const { smokeTestSyncWorker } = await import("@oh-my-pi/omp-stats"); + await smokeTestSyncWorker(); + process.stdout.write("smoke-test: ok\n"); +} + /** Run the CLI with the given argv (no `process.argv` prefix). */ -export function runCli(argv: string[]): Promise { +export async function runCli(argv: string[]): Promise { + if (argv[0] === "--smoke-test") { + await runSmokeTest(); + return; + } // --help and --version are handled by run() directly, don't rewrite those. // Everything else that isn't a known subcommand routes to "launch". const first = argv[0]; diff --git a/packages/coding-agent/src/eval/js/context-manager.ts b/packages/coding-agent/src/eval/js/context-manager.ts index b7a0240b7..c12550039 100644 --- a/packages/coding-agent/src/eval/js/context-manager.ts +++ b/packages/coding-agent/src/eval/js/context-manager.ts @@ -1,14 +1,13 @@ -import { logger, Snowflake } from "@oh-my-pi/pi-utils"; +import { isCompiledBinary, logger, Snowflake } from "@oh-my-pi/pi-utils"; import type { ToolSession } from "../../tools"; import { ToolAbortError, ToolError } from "../../tools/tool-errors"; import { callSessionTool, type JsStatusEvent } from "./tool-bridge"; import { WorkerCore } from "./worker-core"; -// Imported with `type: "file"` so Bun's bundler statically discovers the worker entry and -// embeds it inside `bun build --compile` single-file binaries. Mirrors the browser tab -// worker setup; see packages/coding-agent/src/tools/browser/tab-supervisor.ts for the -// rationale. -// @ts-expect-error -- Bun file-URL import attribute is not modeled by tsgo. -import jsWorkerEntryUrl from "./worker-entry.ts" with { type: "file" }; +// Worker entry. See `tab-supervisor.ts` for the rationale behind the +// literal-string + `new URL(import.meta.url)` hybrid: the literal is what +// Bun's `--compile` bundler discovers, the `new URL` form is what makes dev +// runs portable across cwds. The worker is registered as an additional +// `--compile` entrypoint in `scripts/build-binary.ts`. import type { JsDisplayOutput, RunErrorPayload, @@ -344,7 +343,9 @@ async function raceWithTimeout(promise: Promise, timeoutMs: number, reason async function spawnJsWorker(): Promise { try { - const worker = new Worker(jsWorkerEntryUrl, { type: "module" }); + const worker = isCompiledBinary() + ? new Worker("./packages/coding-agent/src/eval/js/worker-entry.ts", { type: "module" }) + : new Worker(new URL("./worker-entry.ts", import.meta.url).href, { type: "module" }); return wrapBunWorker(worker); } catch (err) { logger.warn("Bun Worker spawn failed; using inline JS eval worker (no sync-loop guard)", { diff --git a/packages/coding-agent/src/tools/browser/tab-supervisor.ts b/packages/coding-agent/src/tools/browser/tab-supervisor.ts index e15db5bb8..16770068d 100644 --- a/packages/coding-agent/src/tools/browser/tab-supervisor.ts +++ b/packages/coding-agent/src/tools/browser/tab-supervisor.ts @@ -1,4 +1,4 @@ -import { getPuppeteerDir, logger, Snowflake } from "@oh-my-pi/pi-utils"; +import { getPuppeteerDir, isCompiledBinary, logger, Snowflake } from "@oh-my-pi/pi-utils"; import type { Page, Target } from "puppeteer-core"; import { callSessionTool } from "../../eval/js/tool-bridge"; import type { ToolSession } from "../../sdk"; @@ -17,17 +17,15 @@ import type { WorkerInitPayload, WorkerOutbound, } from "./tab-protocol"; -// Imported with `type: "file"` so Bun's bundler statically discovers the -// worker entry and embeds it inside `bun build --compile` single-file -// binaries. Without this attribute the bundler cannot reach the entry through -// a `new URL(..., import.meta.url)` literal stored in a local variable, and -// the prebuilt binary surfaces `Timed out initializing browser tab worker` -// (issue #1011) because `/$bunfs/root/tab-worker-entry.ts` is missing. -// tsgo doesn't recognize Bun's `with { type: "file" }` attribute and treats -// this as a normal TS source import, raising TS1192/TS5097. Bun's bundler -// (and runtime) honors the attribute and returns the embedded file URL. -// @ts-expect-error -- Bun file-URL import (see comment above). -import tabWorkerEntryUrl from "./tab-worker-entry.ts" with { type: "file" }; + +// Worker entry. The literal string in `new Worker("./packages/coding-agent/src/tools/browser/tab-worker-entry.ts", …)` +// below is what Bun's `--compile` static analyzer needs to bundle the worker +// (registered as an additional entrypoint in `scripts/build-binary.ts`); in +// dev we resolve the same source via `import.meta.url`. Replaces the older +// `with { type: "file" }` pattern, which only copied the entry as a raw +// asset and could not resolve the worker's relative imports inside a +// compiled binary (issue #1011 was a false-positive fix — the regression +// test only checked emission, not actual worker startup). interface WorkerHandle { send(msg: WorkerInbound, transferList?: Transferable[]): void; @@ -456,7 +454,9 @@ async function raceWithTimeout( async function spawnTabWorker(): Promise { try { - const worker = new Worker(tabWorkerEntryUrl, { type: "module" }); + const worker = isCompiledBinary() + ? new Worker("./packages/coding-agent/src/tools/browser/tab-worker-entry.ts", { type: "module" }) + : new Worker(new URL("./tab-worker-entry.ts", import.meta.url).href, { type: "module" }); return wrapBunWorker(worker); } catch (err) { logger.warn("Bun Worker spawn failed; using inline tab worker (no sync-loop guard)", { diff --git a/packages/stats/src/aggregator.ts b/packages/stats/src/aggregator.ts index c243564f1..610303eee 100644 --- a/packages/stats/src/aggregator.ts +++ b/packages/stats/src/aggregator.ts @@ -1,4 +1,5 @@ import * as fs from "node:fs"; +import { isCompiledBinary } from "@oh-my-pi/pi-utils"; import { getRecentErrors as dbGetRecentErrors, getRecentRequests as dbGetRecentRequests, @@ -23,13 +24,15 @@ import { } from "./db"; import { getSessionEntry, listAllSessionFiles, type ParseSessionResult } from "./parser"; import type { SyncWorkerRequest, SyncWorkerResponse } from "./sync-worker"; -// `with { type: "file" }` resolves to the worker's absolute path at runtime -// (dev) and survives bundling (the asset is copied alongside the build). -// tsgo doesn't recognize Bun's file-URL import attribute and would raise -// TS1192/TS5097 here; Bun honors it. Same suppression pattern lives in -// `tab-supervisor.ts` and `context-manager.ts`. -// @ts-expect-error -- Bun file-URL import (see comment above). -import syncWorkerUrl from "./sync-worker.ts" with { type: "file" }; +// Worker entry. Bun's `--compile` bundler statically discovers the string +// literal in `new Worker("./packages/stats/src/sync-worker.ts", …)` below and +// emits the worker as an additional entrypoint (registered in +// `packages/coding-agent/scripts/build-binary.ts`). In dev runs we resolve +// the same source file through `import.meta.url`, so the literal only has to +// be valid relative to the `--root` directory (repo root). Importing the +// source as `with { type: "file" }` is NOT sufficient — that copies the file +// as a raw asset and does not bundle the worker's relative imports, so the +// worker would crash on first `import` (issue #1011, PR #1027). import type { BehaviorDashboardStats, DashboardStats, MessageStats, RequestDetails } from "./types"; /** @@ -85,8 +88,22 @@ interface WorkerHandle { reject: ((err: Error) => void) | null; } +/** + * Create a fresh sync worker. In a `--compile` binary the literal-string + * specifier is what Bun's static analyzer needs (the file is also listed as + * an additional `--compile` entrypoint in + * `packages/coding-agent/scripts/build-binary.ts`). In dev runs we resolve + * the source URL via `import.meta.url` so the worker survives `cwd` changes + * by callers. + */ +function createSyncWorker(): Worker { + return isCompiledBinary() + ? new Worker("./packages/stats/src/sync-worker.ts", { type: "module" }) + : new Worker(new URL("./sync-worker.ts", import.meta.url).href, { type: "module" }); +} + function spawnWorker(): WorkerHandle { - const worker = new Worker(syncWorkerUrl, { type: "module" }); + const worker = createSyncWorker(); const handle: WorkerHandle = { worker, busy: false, resolve: null, reject: null }; worker.onmessage = (event: MessageEvent) => { const { resolve, reject } = handle; @@ -94,8 +111,16 @@ function spawnWorker(): WorkerHandle { handle.reject = null; handle.busy = false; if (!resolve || !reject) return; - if (event.data.ok) resolve(event.data.result); - else reject(new Error(event.data.error)); + const data = event.data; + if (!data.ok) { + reject(new Error(data.error)); + return; + } + if (data.kind === "pong") { + reject(new Error("sync worker: unexpected pong on parse channel")); + return; + } + resolve(data.result); }; worker.onerror = (event: ErrorEvent) => { const { reject } = handle; @@ -119,6 +144,45 @@ function dispatch(handle: WorkerHandle, request: SyncWorkerRequest): Promise { + const worker = createSyncWorker(); + const { promise, resolve, reject } = Promise.withResolvers(); + const timer = setTimeout(() => reject(new Error(`sync worker did not pong within ${timeoutMs}ms`)), timeoutMs); + worker.onmessage = (event: MessageEvent) => { + const data = event.data; + if (!data.ok) { + reject(new Error(data.error)); + return; + } + if (data.kind !== "pong") { + reject(new Error(`sync worker: expected pong, got ${JSON.stringify(data)}`)); + return; + } + resolve(); + }; + worker.onerror = (event: ErrorEvent) => { + reject(event.error instanceof Error ? event.error : new Error(event.message || "worker error")); + }; + try { + worker.postMessage({ kind: "ping" } satisfies SyncWorkerRequest); + await promise; + } finally { + clearTimeout(timer); + worker.terminate(); + } +} + /** * Sync all session files to the database. * diff --git a/packages/stats/src/index.ts b/packages/stats/src/index.ts index 862308ee6..829da00ef 100755 --- a/packages/stats/src/index.ts +++ b/packages/stats/src/index.ts @@ -11,6 +11,7 @@ export { getTotalMessageCount, type SyncOptions, type SyncProgress, + smokeTestSyncWorker, syncAllSessions, } from "./aggregator"; export { closeDb } from "./db"; diff --git a/packages/stats/src/sync-worker.ts b/packages/stats/src/sync-worker.ts index 52b3d7961..a8247febd 100644 --- a/packages/stats/src/sync-worker.ts +++ b/packages/stats/src/sync-worker.ts @@ -4,25 +4,34 @@ * `parseSessionFile` (which is pure I/O + CPU, no DB), and post the * structured-clone-safe result back. One in-flight request per worker so * the main thread can fan jobs out 1:1 with the pool size. + * + * A `{ kind: "ping" }` request is also accepted and replies with + * `{ ok: true, kind: "pong" }` — used by `smokeTestSyncWorker` to prove the + * worker actually spawns and runs in compiled binaries (regression coverage + * for issue #1011 / PR #1027, where the worker silently failed to load). */ import { type ParseSessionResult, parseSessionFile } from "./parser"; -export interface SyncWorkerRequest { - sessionFile: string; - fromOffset: number; -} +export type SyncWorkerRequest = { kind?: "parse"; sessionFile: string; fromOffset: number } | { kind: "ping" }; -export type SyncWorkerResponse = { ok: true; result: ParseSessionResult } | { ok: false; error: string }; +export type SyncWorkerResponse = + | { ok: true; kind?: "parse"; result: ParseSessionResult } + | { ok: true; kind: "pong" } + | { ok: false; error: string }; declare const self: Worker & { onmessage: ((event: MessageEvent) => void) | null; }; self.onmessage = async event => { - const { sessionFile, fromOffset } = event.data; + const request = event.data; try { - const result = await parseSessionFile(sessionFile, fromOffset); + if (request.kind === "ping") { + self.postMessage({ ok: true, kind: "pong" } satisfies SyncWorkerResponse); + return; + } + const result = await parseSessionFile(request.sessionFile, request.fromOffset); self.postMessage({ ok: true, result } satisfies SyncWorkerResponse); } catch (err) { const error = err instanceof Error ? (err.stack ?? err.message) : String(err); diff --git a/packages/utils/src/env.ts b/packages/utils/src/env.ts index bb77b04df..f2efbac5f 100644 --- a/packages/utils/src/env.ts +++ b/packages/utils/src/env.ts @@ -100,6 +100,20 @@ export function isBunTestRuntime(): boolean { return Bun.env.BUN_ENV === "test" || Bun.env.NODE_ENV === "test"; } +/** + * True when this code is running inside a `bun build --compile` standalone + * binary. Detects via the embedded virtual-filesystem path markers + * (`$bunfs`, `~BUN`, or its URL-encoded form `%7EBUN`) in `import.meta.url`, + * which Bun rewrites for every module bundled into the executable. The + * `PI_COMPILED` env var (set by the build script's `--define`) is checked + * first for cheap fast-path detection. + */ +export function isCompiledBinary(): boolean { + if (Bun.env.PI_COMPILED) return true; + const url = import.meta.url; + return url.includes("$bunfs") || url.includes("~BUN") || url.includes("%7EBUN"); +} + const TRUTHY: Dict = { "1": true, Y: true, TRUE: true, YES: true, ON: true }; export function $flag(name: string, def: boolean = false): boolean { const value = $env[name]; diff --git a/scripts/install-tests/run-ci.sh b/scripts/install-tests/run-ci.sh index 6f96bc1e4..7bcf71ced 100755 --- a/scripts/install-tests/run-ci.sh +++ b/scripts/install-tests/run-ci.sh @@ -21,6 +21,13 @@ smoke_cli() { XDG_DATA_HOME="$runtime_dir/xdg" HOME="$runtime_dir/home" "$omp_bin" --version XDG_DATA_HOME="$runtime_dir/xdg" HOME="$runtime_dir/home" "$omp_bin" --help >/dev/null XDG_DATA_HOME="$runtime_dir/xdg" HOME="$runtime_dir/home" "$omp_bin" stats --summary >/dev/null + # Spawns the stats sync worker via `new Worker(...)` and waits for a pong. + # Regression probe for #1011 (browser tab worker) and #1027 (stats sync + # worker) — both broke silently in compiled binaries because the `with + # { type: "file" }` import pattern only copies the worker as a raw asset + # without bundling its imports. `stats --summary` doesn't catch this on a + # fresh install (no session files = no Worker spawn). + XDG_DATA_HOME="$runtime_dir/xdg" HOME="$runtime_dir/home" "$omp_bin" --smoke-test } find_tarball() {