From 00489a77d6eb978ee1cedfefe0d94427b3036ff0 Mon Sep 17 00:00:00 2001 From: Vlad Temian Date: Thu, 30 Jul 2026 17:00:30 +0300 Subject: [PATCH 1/6] fix: defer bun:sqlite import so non-Bun loaders can resolve the plugin The artifact index imported bun:sqlite statically, and it sits in the eager chain through hooks/artifact-auto-index. Node and Electron reject the bun: scheme while resolving the module graph, so the whole plugin failed to register on OpenCode Desktop and none of its commands ran. Move the specifier behind an await import() at call time and keep the type import, which is erased. The build now emits no static bun: specifier, guarded by a bundle test. Also stop caching a half-built index when initialize() rejects. That path was unreachable while the import could not fail; now it can, and without this every later call reports "Database not initialized" instead of the real cause. Refs #60 --- src/tools/artifact-index/index.ts | 36 ++++++++++++++----- .../integration/bundle-runtime-compat.test.ts | 27 ++++++++++++++ 2 files changed, 55 insertions(+), 8 deletions(-) create mode 100644 tests/integration/bundle-runtime-compat.test.ts diff --git a/src/tools/artifact-index/index.ts b/src/tools/artifact-index/index.ts index 1f4a4cfd..25e44d5f 100644 --- a/src/tools/artifact-index/index.ts +++ b/src/tools/artifact-index/index.ts @@ -1,12 +1,16 @@ // src/tools/artifact-index/index.ts -import { Database } from "bun:sqlite"; + +import type { Database } from "bun:sqlite"; import { existsSync, mkdirSync, readFileSync } from "node:fs"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; +import { extractErrorMessage } from "@/utils/errors"; const DEFAULT_DB_DIR = join(homedir(), ".config", "opencode", "artifact-index"); const DB_NAME = "context.db"; const ERR_DB_NOT_INITIALIZED = "Database not initialized"; +const ERR_SQLITE_UNAVAILABLE = + "Artifact index requires Bun's sqlite, which this runtime does not provide. Plans and ledgers will not be indexed or searchable"; const DEFAULT_SEARCH_LIMIT = 10; export interface PlanRecord { @@ -316,13 +320,26 @@ export interface ArtifactIndex { close(): Promise; } -function initializeDb(dbPath: string): Database { +// Imported at call time so the `bun:` specifier never becomes a static import. +// Node and Electron ESM loaders reject that scheme while resolving the module +// graph, which would take the whole plugin down before it can register. +async function loadSqlite(): Promise { + try { + const sqlite = await import("bun:sqlite"); + return sqlite.Database; + } catch (error) { + throw new Error(`${ERR_SQLITE_UNAVAILABLE}: ${extractErrorMessage(error)}`, { cause: error }); + } +} + +async function initializeDb(dbPath: string): Promise { const dir = dirname(dbPath); if (!existsSync(dir)) { mkdirSync(dir, { recursive: true }); } - const database = new Database(dbPath); + const Sqlite = await loadSqlite(); + const database = new Sqlite(dbPath); const schemaPath = join(dirname(import.meta.path), "schema.sql"); let schema: string; try { @@ -347,7 +364,7 @@ export function createArtifactIndex(dbDir: string = DEFAULT_DB_DIR): ArtifactInd return { async initialize(): Promise { - db = initializeDb(dbPath); + db = await initializeDb(dbPath); }, async indexPlan(record: PlanRecord): Promise { indexPlanInDb(requireDb(db), record); @@ -386,9 +403,12 @@ export function createArtifactIndex(dbDir: string = DEFAULT_DB_DIR): ArtifactInd let globalIndex: ArtifactIndex | null = null; export async function getArtifactIndex(): Promise { - if (!globalIndex) { - globalIndex = createArtifactIndex(); - await globalIndex.initialize(); - } + if (globalIndex) return globalIndex; + + const index = createArtifactIndex(); + // Only cache once initialization succeeds, otherwise every later call gets a + // half-built index reporting "not initialized" instead of the real failure. + await index.initialize(); + globalIndex = index; return globalIndex; } diff --git a/tests/integration/bundle-runtime-compat.test.ts b/tests/integration/bundle-runtime-compat.test.ts new file mode 100644 index 00000000..59729eae --- /dev/null +++ b/tests/integration/bundle-runtime-compat.test.ts @@ -0,0 +1,27 @@ +// tests/integration/bundle-runtime-compat.test.ts +import { describe, expect, it } from "bun:test"; +import { join } from "node:path"; + +const ENTRY = join(import.meta.dir, "../../src/index.ts"); +const STATIC_BUN_IMPORT = /^\s*import[^;]*from\s*["']bun:[a-z]+["']/m; + +async function bundle(): Promise { + const built = await Bun.build({ entrypoints: [ENTRY], target: "bun", external: ["bun-pty"] }); + expect(built.success).toBe(true); + return await built.outputs[0].text(); +} + +describe("bundle runtime compatibility", () => { + // A static `bun:` specifier is rejected by the Node and Electron ESM loaders + // while they resolve the module graph, which takes the whole plugin down + // before it can register. Any such import must be deferred to call time. + it("emits no static bun: import that non-Bun ESM loaders reject", async () => { + const output = await bundle(); + expect(output).not.toMatch(STATIC_BUN_IMPORT); + }); + + it("still reaches bun:sqlite through a deferred import", async () => { + const output = await bundle(); + expect(output).toContain('import("bun:sqlite")'); + }); +}); From c7db5d7057a23990aaeef19b5105492e6b3e9f66 Mon Sep 17 00:00:00 2001 From: Vlad Temian Date: Thu, 30 Jul 2026 17:19:54 +0300 Subject: [PATCH 2/6] refactor: replace Bun process and file APIs with node: equivalents ast-grep and btca imported spawn and which from the bun builtin, which made a node-target build impossible. Both used the same which-then-spawn shape, so extract one runtime-neutral helper over node:child_process and port them onto it. Also swap the remaining Bun globals outside octto's server: Bun.file stat and text, Bun.write, and the browser opener's Bun.spawn all have direct node: counterparts. Refs #60 --- src/hooks/ledger-loader.ts | 6 +-- src/octto/session/browser.ts | 15 ++++-- src/octto/state/persistence.ts | 5 +- src/tools/ast-grep/index.ts | 15 ++---- src/tools/btca/index.ts | 23 ++------- src/utils/process.ts | 91 +++++++++++++++++++++++++++++++++ tests/utils/process.test.ts | 92 ++++++++++++++++++++++++++++++++++ 7 files changed, 205 insertions(+), 42 deletions(-) create mode 100644 src/utils/process.ts create mode 100644 tests/utils/process.test.ts diff --git a/src/hooks/ledger-loader.ts b/src/hooks/ledger-loader.ts index f45d9872..1dffc6c5 100644 --- a/src/hooks/ledger-loader.ts +++ b/src/hooks/ledger-loader.ts @@ -1,6 +1,6 @@ // src/hooks/ledger-loader.ts -import { readdir, readFile } from "node:fs/promises"; +import { readdir, readFile, stat } from "node:fs/promises"; import { join } from "node:path"; import type { PluginInput } from "@opencode-ai/plugin"; import { config } from "@/utils/config"; @@ -13,8 +13,8 @@ export interface LedgerInfo { async function getFileMtime(filePath: string): Promise { try { - const stat = await Bun.file(filePath).stat(); - return stat ? stat.mtime.getTime() : 0; + const stats = await stat(filePath); + return stats.mtime.getTime(); } catch { return 0; } diff --git a/src/octto/session/browser.ts b/src/octto/session/browser.ts index 6c44e000..629d9cc7 100644 --- a/src/octto/session/browser.ts +++ b/src/octto/session/browser.ts @@ -1,6 +1,8 @@ // src/octto/session/browser.ts // Cross-platform browser opener +import { spawn } from "node:child_process"; + /** * Opens the default browser to the specified URL. * Detects platform and uses appropriate command. @@ -23,10 +25,13 @@ export async function openBrowser(url: string): Promise { break; } - const proc = Bun.spawn(command, { - stdout: "ignore", - stderr: "ignore", - }); + const [executable, ...args] = command; - await proc.exited; + await new Promise((resolve, reject) => { + const child = spawn(executable, args, { stdio: "ignore" }); + child.on("error", reject); + child.on("close", () => { + resolve(); + }); + }); } diff --git a/src/octto/state/persistence.ts b/src/octto/state/persistence.ts index f76b842c..6e2eff91 100644 --- a/src/octto/state/persistence.ts +++ b/src/octto/state/persistence.ts @@ -1,5 +1,6 @@ // src/octto/state/persistence.ts import { existsSync, mkdirSync, readdirSync, rmSync } from "node:fs"; +import { readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; import * as v from "valibot"; @@ -54,7 +55,7 @@ export function createStatePersistence(baseDir = STATE_DIR): StatePersistence { ensureDir(); const filePath = getFilePath(state.session_id); state.updated_at = Date.now(); - await Bun.write(filePath, JSON.stringify(state, null, 2)); + await writeFile(filePath, JSON.stringify(state, null, 2), "utf8"); }, async load(sessionId: string): Promise { @@ -62,7 +63,7 @@ export function createStatePersistence(baseDir = STATE_DIR): StatePersistence { if (!existsSync(filePath)) { return null; } - const content = await Bun.file(filePath).text(); + const content = await readFile(filePath, "utf8"); return deserializeState(content, filePath); }, diff --git a/src/tools/ast-grep/index.ts b/src/tools/ast-grep/index.ts index e06022a5..d31ccd72 100644 --- a/src/tools/ast-grep/index.ts +++ b/src/tools/ast-grep/index.ts @@ -1,13 +1,13 @@ import { tool } from "@opencode-ai/plugin/tool"; -import { spawn, which } from "bun"; import * as v from "valibot"; +import { findExecutable, runCommand } from "@/utils/process"; /** * Check if ast-grep CLI (sg) is available on the system. * Returns installation instructions if not found. */ export async function checkAstGrepAvailable(): Promise<{ available: boolean; message?: string }> { - const sgPath = which("sg"); + const sgPath = findExecutable("sg"); if (sgPath) { return { available: true }; } @@ -96,16 +96,7 @@ function parseMatchOutput(stdout: string): SgResult { async function runSg(args: string[]): Promise { try { - const proc = spawn(["sg", ...args], { - stdout: "pipe", - stderr: "pipe", - }); - - const [stdout, stderr, exitCode] = await Promise.all([ - new Response(proc.stdout).text(), - new Response(proc.stderr).text(), - proc.exited, - ]); + const { stdout, stderr, exitCode } = await runCommand("sg", args); const isNoFilesFound = exitCode !== 0 && !stdout.trim() && stderr.includes("No files found"); if (isNoFilesFound) { diff --git a/src/tools/btca/index.ts b/src/tools/btca/index.ts index a8e39c91..d0f1dfce 100644 --- a/src/tools/btca/index.ts +++ b/src/tools/btca/index.ts @@ -1,14 +1,14 @@ import { tool } from "@opencode-ai/plugin/tool"; -import { spawn, which } from "bun"; import { config } from "@/utils/config"; import { extractErrorMessage } from "@/utils/errors"; +import { findExecutable, runCommand } from "@/utils/process"; /** * Check if btca CLI is available on the system. * Returns installation instructions if not found. */ export async function checkBtcaAvailable(): Promise<{ available: boolean; message?: string }> { - const btcaPath = which("btca"); + const btcaPath = findExecutable("btca"); if (btcaPath) { return { available: true }; } @@ -23,24 +23,7 @@ export async function checkBtcaAvailable(): Promise<{ available: boolean; messag async function runBtca(args: string[]): Promise<{ output: string; error?: string }> { try { - const proc = spawn(["btca", ...args], { - stdout: "pipe", - stderr: "pipe", - }); - - // Create timeout promise - const timeoutPromise = new Promise((_, reject) => { - setTimeout(() => { - proc.kill(); - reject(new Error("btca command timed out after 2 minutes")); - }, config.timeouts.btcaMs); - }); - - // Race between process completion and timeout - const [stdout, stderr, exitCode] = await Promise.race([ - Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text(), proc.exited]), - timeoutPromise, - ]); + const { stdout, stderr, exitCode } = await runCommand("btca", args, { timeoutMs: config.timeouts.btcaMs }); if (exitCode !== 0) { const errorMsg = stderr.trim() || `Exit code ${exitCode}`; diff --git a/src/utils/process.ts b/src/utils/process.ts new file mode 100644 index 00000000..a92f35ff --- /dev/null +++ b/src/utils/process.ts @@ -0,0 +1,91 @@ +// src/utils/process.ts +// Runtime-neutral process helpers. Bun's `which` and `spawn` are unavailable +// under Node and Electron, which host OpenCode Desktop, so these wrap the +// node: equivalents that both runtimes provide. + +import { spawn } from "node:child_process"; +import { accessSync, constants } from "node:fs"; +import { delimiter, join } from "node:path"; + +const WINDOWS_DEFAULT_EXTENSIONS = ".COM;.EXE;.BAT;.CMD"; +const TIMEOUT_SIGNAL = "SIGTERM"; + +export interface CommandResult { + readonly stdout: string; + readonly stderr: string; + readonly exitCode: number; +} + +function isExecutable(candidate: string): boolean { + try { + accessSync(candidate, constants.X_OK); + return true; + } catch { + // Not present or not executable at this location; keep scanning PATH. + return false; + } +} + +function executableExtensions(): string[] { + if (process.platform !== "win32") return [""]; + return (process.env.PATHEXT ?? WINDOWS_DEFAULT_EXTENSIONS).split(";").filter(Boolean); +} + +function findInDirectory(dir: string, name: string): string | null { + for (const extension of executableExtensions()) { + const candidate = join(dir, `${name}${extension}`); + if (isExecutable(candidate)) return candidate; + } + return null; +} + +/** + * Resolve an executable through PATH, returning its full path or null. + */ +export function findExecutable(name: string): string | null { + const searchPaths = (process.env.PATH ?? "").split(delimiter).filter(Boolean); + + for (const dir of searchPaths) { + const found = findInDirectory(dir, name); + if (found) return found; + } + return null; +} + +/** + * Run a command to completion, collecting stdout and stderr. + * + * Rejects if the binary cannot be spawned, or if timeoutMs elapses first. + */ +export function runCommand( + command: string, + args: string[], + options: { timeoutMs?: number } = {}, +): Promise { + return new Promise((resolve, reject) => { + const child = spawn(command, args, { + stdio: ["ignore", "pipe", "pipe"], + ...(options.timeoutMs === undefined ? {} : { timeout: options.timeoutMs, killSignal: TIMEOUT_SIGNAL }), + }); + + let stdout = ""; + let stderr = ""; + child.stdout.setEncoding("utf8"); + child.stderr.setEncoding("utf8"); + child.stdout.on("data", (chunk: string) => { + stdout += chunk; + }); + child.stderr.on("data", (chunk: string) => { + stderr += chunk; + }); + + child.on("error", reject); + child.on("close", (code, signal) => { + if (signal === TIMEOUT_SIGNAL && options.timeoutMs !== undefined) { + reject(new Error(`${command} timed out after ${options.timeoutMs}ms`)); + return; + } + resolve({ stdout, stderr, exitCode: code ?? 0 }); + }); + }); +} diff --git a/tests/utils/process.test.ts b/tests/utils/process.test.ts new file mode 100644 index 00000000..7744d38e --- /dev/null +++ b/tests/utils/process.test.ts @@ -0,0 +1,92 @@ +// tests/utils/process.test.ts +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { delimiter, join } from "node:path"; + +import { findExecutable, runCommand } from "../../src/utils/process"; + +describe("findExecutable", () => { + let binDir: string; + let originalPath: string | undefined; + + beforeEach(() => { + binDir = mkdtempSync(join(tmpdir(), "process-util-test-")); + originalPath = process.env.PATH; + }); + + afterEach(() => { + rmSync(binDir, { recursive: true, force: true }); + if (originalPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = originalPath; + } + }); + + it("resolves an executable on PATH to its full path", () => { + const binary = join(binDir, "fake-tool"); + writeFileSync(binary, "#!/bin/sh\nexit 0\n"); + chmodSync(binary, 0o755); + process.env.PATH = binDir; + + expect(findExecutable("fake-tool")).toBe(binary); + }); + + it("returns null when the name is absent from PATH", () => { + process.env.PATH = binDir; + expect(findExecutable("definitely-not-installed")).toBeNull(); + }); + + it("ignores a non-executable file of the same name", () => { + const binary = join(binDir, "not-runnable"); + writeFileSync(binary, "plain text"); + chmodSync(binary, 0o644); + process.env.PATH = binDir; + + expect(findExecutable("not-runnable")).toBeNull(); + }); + + it("searches every PATH entry in order", () => { + const second = mkdtempSync(join(tmpdir(), "process-util-second-")); + const binary = join(second, "later-tool"); + writeFileSync(binary, "#!/bin/sh\nexit 0\n"); + chmodSync(binary, 0o755); + process.env.PATH = [binDir, second].join(delimiter); + + expect(findExecutable("later-tool")).toBe(binary); + rmSync(second, { recursive: true, force: true }); + }); + + it("returns null when PATH is unset", () => { + delete process.env.PATH; + expect(findExecutable("sh")).toBeNull(); + }); +}); + +describe("runCommand", () => { + it("collects stdout and a zero exit code", async () => { + const result = await runCommand("sh", ["-c", "printf hello"]); + expect(result.stdout).toBe("hello"); + expect(result.exitCode).toBe(0); + }); + + it("collects stderr and a non-zero exit code without throwing", async () => { + const result = await runCommand("sh", ["-c", "printf oops >&2; exit 3"]); + expect(result.stderr).toBe("oops"); + expect(result.exitCode).toBe(3); + }); + + it("rejects when the binary cannot be spawned", async () => { + await expect(runCommand("definitely-not-installed", [])).rejects.toThrow(); + }); + + it("rejects when the command outlives its timeout", async () => { + await expect(runCommand("sh", ["-c", "sleep 5"], { timeoutMs: 50 })).rejects.toThrow(/timed out after 50ms/); + }); + + it("does not apply a timeout when none is given", async () => { + const result = await runCommand("sh", ["-c", "printf done"]); + expect(result.stdout).toBe("done"); + }); +}); From bd001375d4b94ae0760b0fec1319f653c3a74f7c Mon Sep 17 00:00:00 2001 From: Vlad Temian Date: Fri, 31 Jul 2026 10:50:19 +0300 Subject: [PATCH 3/6] feat: run octto's server on node:http so the plugin works off Bun Bun.serve was the last runtime dependency keeping the plugin from working under Electron. Replace it with node:http plus ws, which both Bun and Node run, and switch the shipped build to --target node so the bundle stops emitting the Bun-only import.meta.require shim. Octto only ever needed send() from a socket and stop()/port/hostname from a server, so express that as SessionSocket and SessionServer rather than leaking a runtime's types through the session layer. jsonc-parser joins bun-pty as external: its UMD build resolves nested requires that do not survive node-target bundling. Adds the first coverage for this module, driving a real server with real client sockets over loopback. Closes #60 --- bun.lock | 6 + package.json | 4 +- src/octto/session/server.ts | 190 +++++++++--------- src/octto/session/sessions.ts | 6 +- src/octto/session/types.ts | 20 +- .../integration/bundle-runtime-compat.test.ts | 15 +- tests/octto/server.test.ts | 174 ++++++++++++++++ 7 files changed, 308 insertions(+), 107 deletions(-) create mode 100644 tests/octto/server.test.ts diff --git a/bun.lock b/bun.lock index 912702a1..94457164 100644 --- a/bun.lock +++ b/bun.lock @@ -9,11 +9,13 @@ "bun-pty": "^0.4.5", "jsonc-parser": "^3.3.1", "valibot": "^1.2.0", + "ws": "^8.21.1", "yaml": "^2.8.2", }, "devDependencies": { "@biomejs/biome": "^2.3.10", "@eslint/js": "^10.0.1", + "@types/ws": "^8.18.1", "bun-types": "latest", "eslint": "^10.8.0", "eslint-plugin-sonarjs": "^4.0.2", @@ -97,6 +99,8 @@ "@types/node": ["@types/node@25.0.3", "", { "dependencies": { "undici-types": "~7.16.0" } }, "sha512-W609buLVRVmeW693xKfzHeIV6nJGGz98uCPfeXI1ELMLXVeKYZ9m15fAMSaUPBHYLGFsVRcMmSCksQOrZV9BYA=="], + "@types/ws": ["@types/ws@8.18.1", "", { "dependencies": { "@types/node": "*" } }, "sha512-ThVF6DCVhA8kUGy+aazFQ4kXQ7E1Ty7A3ypFOe0IcJV8O/M511G99AW24irKrW56Wt44yG9+ij8FaqoBGkuBXg=="], + "@typescript-eslint/eslint-plugin": ["@typescript-eslint/eslint-plugin@8.65.0", "", { "dependencies": { "@eslint-community/regexpp": "^4.12.2", "@typescript-eslint/scope-manager": "8.65.0", "@typescript-eslint/type-utils": "8.65.0", "@typescript-eslint/utils": "8.65.0", "@typescript-eslint/visitor-keys": "8.65.0", "ignore": "^7.0.5", "natural-compare": "^1.4.0", "ts-api-utils": "^2.5.0" }, "peerDependencies": { "@typescript-eslint/parser": "^8.65.0", "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", "typescript": ">=4.8.4 <6.1.0" } }, "sha512-IEgob78X12rHpUmtcwFsXhZdVGJtwTVP8FiCLZkR6GlYVrl2PcuB+KhCE5BlVC/eQpQnu8WXRtkHZuPar+gCRA=="], "@typescript-eslint/parser": ["@typescript-eslint/parser@8.65.0", "", { "dependencies": { "@typescript-eslint/scope-manager": "8.65.0", "@typescript-eslint/types": "8.65.0", "@typescript-eslint/typescript-estree": "8.65.0", "@typescript-eslint/visitor-keys": "8.65.0", "debug": "^4.4.3" }, "peerDependencies": { "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", "typescript": ">=4.8.4 <6.1.0" } }, "sha512-CZ4nMxWwgu1HEEFNkeaCptra9QCtkmKdgf3sWh1rl1trIhmxLilgTV4cwcbQ4wemnT4sWQN8CaKOmdYx+g2gMA=="], @@ -387,6 +391,8 @@ "word-wrap": ["word-wrap@1.2.5", "", {}, "sha512-BN22B5eaMMI9UMtjrGd5g5eCYPpCPDUy0FJXbYsaT5zYxjFOckS53SQDE3pWkVoWpHXVb3BrYcEN4Twa55B5cA=="], + "ws": ["ws@8.21.1", "", { "peerDependencies": { "bufferutil": "^4.0.1", "utf-8-validate": ">=5.0.2" }, "optionalPeers": ["bufferutil", "utf-8-validate"] }, "sha512-+0NTnW77fFN/DjQi6k/Sq/Yvk4Sgajw7urW8V+asjXnRgDs9gyGkdb7EzgfhA4goXsRIZKE28fzIXBHEzhuiWw=="], + "yaml": ["yaml@2.9.0", "", { "bin": { "yaml": "bin.mjs" } }, "sha512-2AvhNX3mb8zd6Zy7INTtSpl1F15HW6Wnqj0srWlkKLcpYl/gMIMJiyuGq2KeI2YFxUPjdlB+3Lc10seMLtL4cA=="], "yocto-queue": ["yocto-queue@0.1.0", "", {}, "sha512-rVksvsnNCdJ/ohGc6xgPwyN8eheCxsiLM8mxuE/t/mOVqJewPuO1miLpTHQiRgTKCLexL4MeAFVagts7HmNZ2Q=="], diff --git a/package.json b/package.json index d30b3e1b..86484acf 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ ], "scripts": { "prepare": "lefthook install", - "build": "bun build src/index.ts --outdir dist --target bun --external bun-pty", + "build": "bun build src/index.ts --outdir dist --target node --external bun-pty --external jsonc-parser", "typecheck": "tsc --noEmit", "prepublishOnly": "bun run check && bun run build", "test": "bun test", @@ -47,11 +47,13 @@ "bun-pty": "^0.4.5", "jsonc-parser": "^3.3.1", "valibot": "^1.2.0", + "ws": "^8.21.1", "yaml": "^2.8.2" }, "devDependencies": { "@biomejs/biome": "^2.3.10", "@eslint/js": "^10.0.1", + "@types/ws": "^8.18.1", "bun-types": "latest", "eslint": "^10.8.0", "eslint-plugin-sonarjs": "^4.0.2", diff --git a/src/octto/session/server.ts b/src/octto/session/server.ts index 24c5a877..3404514e 100644 --- a/src/octto/session/server.ts +++ b/src/octto/session/server.ts @@ -1,125 +1,123 @@ // src/octto/session/server.ts -import type { Server, ServerWebSocket } from "bun"; +import { createServer as createHttpServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; import * as v from "valibot"; +import { type WebSocket, WebSocketServer } from "ws"; import { getHtmlBundle } from "@/octto/ui"; import { config } from "@/utils/config"; import { extractErrorMessage } from "@/utils/errors"; import { log } from "@/utils/logger"; import { WsClientMessageSchema } from "./schemas"; import type { SessionStore } from "./sessions"; -import type { WsClientMessage } from "./types"; +import type { SessionServer, SessionSocket, WsClientMessage } from "./types"; -interface WsData { - sessionId: string; -} - -export async function createServer( - sessionId: string, - store: SessionStore, -): Promise<{ server: Server; port: number }> { - const htmlBundle = getHtmlBundle(); - - const server = Bun.serve({ - port: 0, // Random available port - hostname: config.octto.allowRemoteBind ? config.octto.bindAddress : "127.0.0.1", - fetch(req, server) { - return handleFetch(req, server, sessionId, htmlBundle); - }, - websocket: createWebSocketHandlers(store), - }); - - // Port is always defined when using port: 0 - const port = server.port; - if (port === undefined) { - throw new Error("Failed to get server port"); - } +const WS_PATH = "/ws"; +const HTML_PATHS = new Set(["/", "/index.html"]); +const LOOPBACK = "127.0.0.1"; +const LOG_MODULE = "octto"; +const ERR_NO_PORT = "Failed to get server port"; +const STATUS_OK = 200; +const STATUS_NOT_FOUND = 404; - return { - server, - port, - }; -} +function serveHttp(req: IncomingMessage, res: ServerResponse, htmlBundle: string): void { + const path = (req.url ?? "").split("?")[0]; -function handleFetch( - req: Request, - server: Server, - sessionId: string, - htmlBundle: string, -): Response | undefined { - const url = new URL(req.url); - - // WebSocket upgrade - if (url.pathname === "/ws") { - const success = server.upgrade(req, { - data: { sessionId }, - }); - if (success) { - return undefined; - } - return new Response("WebSocket upgrade failed", { status: 400 }); - } - - // Serve the bundled HTML app - if (url.pathname === "/" || url.pathname === "/index.html") { - return new Response(htmlBundle, { - headers: { - "Content-Type": "text/html; charset=utf-8", - }, - }); + if (HTML_PATHS.has(path)) { + res.writeHead(STATUS_OK, { "Content-Type": "text/html; charset=utf-8" }); + res.end(htmlBundle); + return; } - return new Response("Not Found", { status: 404 }); + res.writeHead(STATUS_NOT_FOUND, { "Content-Type": "text/plain; charset=utf-8" }); + res.end("Not Found"); } -function createWebSocketHandlers(store: SessionStore): { - open: (ws: ServerWebSocket) => void; - close: (ws: ServerWebSocket) => void; - message: (ws: ServerWebSocket, message: string | Buffer) => void; -} { - return { - open(ws: ServerWebSocket) { - store.handleWsConnect(ws.data.sessionId, ws); - }, - close(ws: ServerWebSocket) { - store.handleWsDisconnect(ws.data.sessionId); - }, - message(ws: ServerWebSocket, message: string | Buffer) { - handleWsMessage(ws, message, store); - }, - }; +function sendError(socket: SessionSocket, error: string, details: string): void { + socket.send(JSON.stringify({ type: "error", error, details })); } -function handleWsMessage(ws: ServerWebSocket, message: string | Buffer, store: SessionStore): void { - const { sessionId } = ws.data; - - let raw: unknown; +function handleWsMessage(socket: SessionSocket, sessionId: string, raw: string, store: SessionStore): void { + let parsed: unknown; try { - raw = JSON.parse(message.toString()); + parsed = JSON.parse(raw); } catch (error) { - log.error("octto", "Failed to parse WebSocket message", error); - ws.send( - JSON.stringify({ - type: "error", - error: "Invalid message format", - details: extractErrorMessage(error), - }), - ); + log.error(LOG_MODULE, "Failed to parse WebSocket message", error); + sendError(socket, "Invalid message format", extractErrorMessage(error)); return; } - const result = v.safeParse(WsClientMessageSchema, raw); + const result = v.safeParse(WsClientMessageSchema, parsed); if (!result.success) { - log.error("octto", "Invalid WebSocket message schema", result.issues); - ws.send( - JSON.stringify({ - type: "error", - error: "Invalid message schema", - details: result.issues.map((i) => i.message).join("; "), - }), - ); + log.error(LOG_MODULE, "Invalid WebSocket message schema", result.issues); + sendError(socket, "Invalid message schema", result.issues.map((issue) => issue.message).join("; ")); return; } store.handleWsMessage(sessionId, result.output as WsClientMessage); } + +function attachWebSockets(wss: WebSocketServer, sessionId: string, store: SessionStore): void { + wss.on("connection", (socket: WebSocket) => { + store.handleWsConnect(sessionId, socket); + + socket.on("message", (data: Buffer | string) => { + handleWsMessage(socket, sessionId, data.toString(), store); + }); + socket.on("close", () => { + store.handleWsDisconnect(sessionId); + }); + socket.on("error", (error: unknown) => { + log.error(LOG_MODULE, "WebSocket connection error", error); + }); + }); +} + +function listen(http: Server, hostname: string): Promise { + return new Promise((resolve, reject) => { + http.once("error", reject); + http.listen(0, hostname, () => { + const address = http.address(); + if (address === null || typeof address === "string") { + reject(new Error(ERR_NO_PORT)); + return; + } + resolve(address.port); + }); + }); +} + +// Live sockets keep node:http from closing, so drop them before waiting. +function stop(http: Server, wss: WebSocketServer): Promise { + return new Promise((resolve) => { + for (const client of wss.clients) { + client.terminate(); + } + wss.close(() => { + http.closeAllConnections(); + http.close(() => { + resolve(); + }); + }); + }); +} + +export async function createServer( + sessionId: string, + store: SessionStore, +): Promise<{ server: SessionServer; port: number }> { + const htmlBundle = getHtmlBundle(); + const hostname = config.octto.allowRemoteBind ? config.octto.bindAddress : LOOPBACK; + + const http = createHttpServer((req, res) => { + serveHttp(req, res, htmlBundle); + }); + const wss = new WebSocketServer({ server: http, path: WS_PATH }); + attachWebSockets(wss, sessionId, store); + + const port = await listen(http, hostname); + + return { + port, + server: { port, hostname, stop: () => stop(http, wss) }, + }; +} diff --git a/src/octto/session/sessions.ts b/src/octto/session/sessions.ts index c2cf18bc..8685a0ea 100644 --- a/src/octto/session/sessions.ts +++ b/src/octto/session/sessions.ts @@ -1,5 +1,4 @@ // src/octto/session/sessions.ts -import type { ServerWebSocket } from "bun"; import { DEFAULT_ANSWER_TIMEOUT_MS } from "@/octto/constants"; import { log } from "@/utils/logger"; @@ -18,6 +17,7 @@ import { type Question, type QuestionType, type Session, + type SessionSocket, STATUSES, type StartSessionInput, type StartSessionOutput, @@ -41,7 +41,7 @@ export interface SessionStore { getNextAnswer: (input: GetNextAnswerInput) => Promise; cancelQuestion: (questionId: string) => { ok: boolean }; listQuestions: (sessionId?: string) => ListQuestionsOutput; - handleWsConnect: (sessionId: string, ws: ServerWebSocket) => void; + handleWsConnect: (sessionId: string, ws: SessionSocket) => void; handleWsDisconnect: (sessionId: string) => void; handleWsMessage: (sessionId: string, message: WsClientMessage) => void; getSession: (sessionId: string) => Session | undefined; @@ -364,7 +364,7 @@ function collectQuestions(sessions: Map, sessionId?: string): L return { questions }; } -function onWsConnect(sessions: Map, sessionId: string, ws: ServerWebSocket): void { +function onWsConnect(sessions: Map, sessionId: string, ws: SessionSocket): void { const session = sessions.get(sessionId); if (!session) return; diff --git a/src/octto/session/types.ts b/src/octto/session/types.ts index 422d74bc..f2792bea 100644 --- a/src/octto/session/types.ts +++ b/src/octto/session/types.ts @@ -1,6 +1,5 @@ // src/octto/session/types.ts // Session and Question types for the octto module -import type { ServerWebSocket } from "bun"; import type { AskCodeConfig, @@ -188,6 +187,21 @@ export type BaseConfig = [key: string]: unknown; }; +/** + * The websocket surface octto needs from whichever server runtime is hosting + * it. Kept minimal so the implementation can be Bun's or Node's. + */ +export interface SessionSocket { + send: (data: string) => void; +} + +/** The HTTP server surface octto needs, independent of runtime. */ +export interface SessionServer { + readonly port: number; + readonly hostname: string; + stop: () => Promise; +} + export interface Session { readonly id: string; readonly title?: string; @@ -196,8 +210,8 @@ export interface Session { readonly createdAt: Date; readonly questions: Map; wsConnected: boolean; - readonly server?: ReturnType; - wsClient?: ServerWebSocket; + readonly server?: SessionServer; + wsClient?: SessionSocket; } export interface InitialQuestion { diff --git a/tests/integration/bundle-runtime-compat.test.ts b/tests/integration/bundle-runtime-compat.test.ts index 59729eae..97d1cb81 100644 --- a/tests/integration/bundle-runtime-compat.test.ts +++ b/tests/integration/bundle-runtime-compat.test.ts @@ -3,23 +3,30 @@ import { describe, expect, it } from "bun:test"; import { join } from "node:path"; const ENTRY = join(import.meta.dir, "../../src/index.ts"); +const EXTERNAL = ["bun-pty", "jsonc-parser"]; const STATIC_BUN_IMPORT = /^\s*import[^;]*from\s*["']bun:[a-z]+["']/m; +// Mirrors the shipped build in package.json. OpenCode Desktop runs on +// Electron, so the published bundle has to resolve and execute under Node. async function bundle(): Promise { - const built = await Bun.build({ entrypoints: [ENTRY], target: "bun", external: ["bun-pty"] }); + const built = await Bun.build({ entrypoints: [ENTRY], target: "node", external: EXTERNAL }); expect(built.success).toBe(true); return await built.outputs[0].text(); } describe("bundle runtime compatibility", () => { - // A static `bun:` specifier is rejected by the Node and Electron ESM loaders - // while they resolve the module graph, which takes the whole plugin down - // before it can register. Any such import must be deferred to call time. it("emits no static bun: import that non-Bun ESM loaders reject", async () => { const output = await bundle(); expect(output).not.toMatch(STATIC_BUN_IMPORT); }); + // import.meta.require exists only in Bun; Node leaves it undefined and the + // generated __require shim throws on first call. + it("emits no import.meta.require shim", async () => { + const output = await bundle(); + expect(output).not.toContain("import.meta.require"); + }); + it("still reaches bun:sqlite through a deferred import", async () => { const output = await bundle(); expect(output).toContain('import("bun:sqlite")'); diff --git a/tests/octto/server.test.ts b/tests/octto/server.test.ts new file mode 100644 index 00000000..438e12a6 --- /dev/null +++ b/tests/octto/server.test.ts @@ -0,0 +1,174 @@ +// tests/octto/server.test.ts +// Exercises the real HTTP + WebSocket server against real client connections. +import { afterEach, describe, expect, it } from "bun:test"; + +import { createServer } from "../../src/octto/session/server"; +import type { SessionServer, SessionSocket, WsClientMessage } from "../../src/octto/session/types"; +import { captureLogs, type LogCapture } from "../helpers/log-capture"; + +const SESSION_ID = "test-session"; + +interface Recorded { + connects: SessionSocket[]; + disconnects: string[]; + messages: WsClientMessage[]; +} + +function createRecordingStore(): { store: Parameters[1]; recorded: Recorded } { + const recorded: Recorded = { connects: [], disconnects: [], messages: [] }; + const store = { + handleWsConnect: (_sessionId: string, socket: SessionSocket) => { + recorded.connects.push(socket); + }, + handleWsDisconnect: (sessionId: string) => { + recorded.disconnects.push(sessionId); + }, + handleWsMessage: (_sessionId: string, message: WsClientMessage) => { + recorded.messages.push(message); + }, + }; + return { store: store as unknown as Parameters[1], recorded }; +} + +function waitFor(predicate: () => boolean, timeoutMs = 2000): Promise { + const started = Date.now(); + return new Promise((resolve, reject) => { + const tick = (): void => { + if (predicate()) { + resolve(); + return; + } + if (Date.now() - started > timeoutMs) { + reject(new Error("condition not met in time")); + return; + } + setTimeout(tick, 10); + }; + tick(); + }); +} + +function connect(port: number): Promise { + const socket = new WebSocket(`ws://127.0.0.1:${port}/ws`); + return new Promise((resolve, reject) => { + socket.addEventListener("open", () => { + resolve(socket); + }); + socket.addEventListener("error", () => { + reject(new Error("websocket failed to open")); + }); + }); +} + +describe("octto session server", () => { + let server: SessionServer | undefined; + let logs: LogCapture | undefined; + + afterEach(async () => { + logs?.restore(); + logs = undefined; + await server?.stop(); + server = undefined; + }); + + it("serves the html bundle on / and 404s unknown paths", async () => { + const { store } = createRecordingStore(); + const started = await createServer(SESSION_ID, store); + server = started.server; + + const page = await fetch(`http://127.0.0.1:${started.port}/`); + expect(page.status).toBe(200); + expect(page.headers.get("content-type")).toContain("text/html"); + expect((await page.text()).length).toBeGreaterThan(0); + + const missing = await fetch(`http://127.0.0.1:${started.port}/nope`); + expect(missing.status).toBe(404); + }); + + it("reports a real bound port", async () => { + const { store } = createRecordingStore(); + const started = await createServer(SESSION_ID, store); + server = started.server; + + expect(started.port).toBeGreaterThan(0); + expect(started.server.hostname).toBe("127.0.0.1"); + }); + + it("routes a websocket connect, message and disconnect to the store", async () => { + const { store, recorded } = createRecordingStore(); + const started = await createServer(SESSION_ID, store); + server = started.server; + + const client = await connect(started.port); + await waitFor(() => recorded.connects.length === 1); + + client.send(JSON.stringify({ type: "response", id: "q1", answer: { value: "yes" } })); + await waitFor(() => recorded.messages.length === 1); + expect(recorded.messages[0]).toMatchObject({ type: "response", id: "q1" }); + + client.close(); + await waitFor(() => recorded.disconnects.length === 1); + expect(recorded.disconnects[0]).toBe(SESSION_ID); + }); + + it("answers malformed json with an error frame instead of dropping the socket", async () => { + logs = captureLogs(); + const { store, recorded } = createRecordingStore(); + const started = await createServer(SESSION_ID, store); + server = started.server; + + const client = await connect(started.port); + await waitFor(() => recorded.connects.length === 1); + + const received: string[] = []; + client.addEventListener("message", (event: MessageEvent) => { + received.push(String(event.data)); + }); + + client.send("not json at all"); + await waitFor(() => received.length === 1); + + expect(JSON.parse(received[0])).toMatchObject({ type: "error", error: "Invalid message format" }); + expect(recorded.messages).toHaveLength(0); + expect(logs.error.some((line) => line.includes("Failed to parse WebSocket message"))).toBe(true); + + client.close(); + }); + + it("rejects a well-formed message that fails schema validation", async () => { + logs = captureLogs(); + const { store, recorded } = createRecordingStore(); + const started = await createServer(SESSION_ID, store); + server = started.server; + + const client = await connect(started.port); + await waitFor(() => recorded.connects.length === 1); + + const received: string[] = []; + client.addEventListener("message", (event: MessageEvent) => { + received.push(String(event.data)); + }); + + client.send(JSON.stringify({ type: "definitely-not-a-real-message" })); + await waitFor(() => received.length === 1); + + expect(JSON.parse(received[0])).toMatchObject({ type: "error", error: "Invalid message schema" }); + expect(recorded.messages).toHaveLength(0); + + client.close(); + }); + + it("stops cleanly while a client is still connected", async () => { + const { store, recorded } = createRecordingStore(); + const started = await createServer(SESSION_ID, store); + + const client = await connect(started.port); + await waitFor(() => recorded.connects.length === 1); + + await started.server.stop(); + server = undefined; + + await expect(fetch(`http://127.0.0.1:${started.port}/`)).rejects.toThrow(); + client.close(); + }); +}); From 6ea91f359c1ba3ec89bf1514bda1a63ce0afed0f Mon Sep 17 00:00:00 2001 From: Vlad Temian Date: Fri, 31 Jul 2026 10:53:07 +0300 Subject: [PATCH 4/6] fix: enforce the command timeout with an explicit timer Bun and Node do not honour the spawn timeout option identically, so the timeout test passed locally and hung for the full sleep on CI. Drive the kill from our own timer instead of trusting the runtime's. --- src/utils/process.ts | 44 ++++++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/src/utils/process.ts b/src/utils/process.ts index a92f35ff..7a7158fa 100644 --- a/src/utils/process.ts +++ b/src/utils/process.ts @@ -6,6 +6,7 @@ import { spawn } from "node:child_process"; import { accessSync, constants } from "node:fs"; import { delimiter, join } from "node:path"; +import type { Readable } from "node:stream"; const WINDOWS_DEFAULT_EXTENSIONS = ".COM;.EXE;.BAT;.CMD"; const TIMEOUT_SIGNAL = "SIGTERM"; @@ -52,37 +53,56 @@ export function findExecutable(name: string): string | null { return null; } +function collect(stream: Readable, append: (chunk: string) => void): void { + stream.setEncoding("utf8"); + stream.on("data", append); +} + /** * Run a command to completion, collecting stdout and stderr. * * Rejects if the binary cannot be spawned, or if timeoutMs elapses first. + * + * The timeout is enforced with an explicit timer rather than the spawn option + * of the same name, which Bun and Node do not honour identically. */ export function runCommand( command: string, args: string[], options: { timeoutMs?: number } = {}, ): Promise { + const { timeoutMs } = options; + return new Promise((resolve, reject) => { - const child = spawn(command, args, { - stdio: ["ignore", "pipe", "pipe"], - ...(options.timeoutMs === undefined ? {} : { timeout: options.timeoutMs, killSignal: TIMEOUT_SIGNAL }), - }); + const child = spawn(command, args, { stdio: ["ignore", "pipe", "pipe"] }); let stdout = ""; let stderr = ""; - child.stdout.setEncoding("utf8"); - child.stderr.setEncoding("utf8"); - child.stdout.on("data", (chunk: string) => { + let timedOut = false; + + const timer = + timeoutMs === undefined + ? undefined + : setTimeout(() => { + timedOut = true; + child.kill(TIMEOUT_SIGNAL); + }, timeoutMs); + + collect(child.stdout, (chunk) => { stdout += chunk; }); - child.stderr.on("data", (chunk: string) => { + collect(child.stderr, (chunk) => { stderr += chunk; }); - child.on("error", reject); - child.on("close", (code, signal) => { - if (signal === TIMEOUT_SIGNAL && options.timeoutMs !== undefined) { - reject(new Error(`${command} timed out after ${options.timeoutMs}ms`)); + child.on("error", (error) => { + clearTimeout(timer); + reject(error); + }); + child.on("close", (code) => { + clearTimeout(timer); + if (timedOut) { + reject(new Error(`${command} timed out after ${timeoutMs}ms`)); return; } resolve({ stdout, stderr, exitCode: code ?? 0 }); From faf1b1f4ac76dfd83f01002958cac0042ab4ec2f Mon Sep 17 00:00:00 2001 From: Vlad Temian Date: Fri, 31 Jul 2026 10:55:19 +0300 Subject: [PATCH 5/6] fix: reject a timed-out command without waiting on its stdio Killing the shell does not necessarily close the pipes: an orphaned grandchild keeps them open, so "close" only arrived once the original command would have finished anyway. CI hit Bun's five second per-test limit as a result. Settle from the timer instead, and assert the elapsed time in the test so the delay cannot come back unnoticed. --- src/utils/process.ts | 9 +++++---- tests/utils/process.test.ts | 8 ++++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/utils/process.ts b/src/utils/process.ts index 7a7158fa..dbe21edd 100644 --- a/src/utils/process.ts +++ b/src/utils/process.ts @@ -80,12 +80,16 @@ export function runCommand( let stderr = ""; let timedOut = false; + // Settle from the timer rather than waiting for "close": a killed shell can + // leave a grandchild holding the stdio pipes open, which delays that event + // for as long as the original command would have run. const timer = timeoutMs === undefined ? undefined : setTimeout(() => { timedOut = true; child.kill(TIMEOUT_SIGNAL); + reject(new Error(`${command} timed out after ${timeoutMs}ms`)); }, timeoutMs); collect(child.stdout, (chunk) => { @@ -101,10 +105,7 @@ export function runCommand( }); child.on("close", (code) => { clearTimeout(timer); - if (timedOut) { - reject(new Error(`${command} timed out after ${timeoutMs}ms`)); - return; - } + if (timedOut) return; resolve({ stdout, stderr, exitCode: code ?? 0 }); }); }); diff --git a/tests/utils/process.test.ts b/tests/utils/process.test.ts index 7744d38e..fd6f4dba 100644 --- a/tests/utils/process.test.ts +++ b/tests/utils/process.test.ts @@ -81,8 +81,12 @@ describe("runCommand", () => { await expect(runCommand("definitely-not-installed", [])).rejects.toThrow(); }); - it("rejects when the command outlives its timeout", async () => { - await expect(runCommand("sh", ["-c", "sleep 5"], { timeoutMs: 50 })).rejects.toThrow(/timed out after 50ms/); + it("rejects as soon as the timeout elapses, not when the command would end", async () => { + const started = Date.now(); + await expect(runCommand("sh", ["-c", "sleep 30"], { timeoutMs: 50 })).rejects.toThrow(/timed out after 50ms/); + // Guards the regression where a grandchild held the pipes open and the + // rejection waited for the full sleep. + expect(Date.now() - started).toBeLessThan(2000); }); it("does not apply a timeout when none is given", async () => { From 7931040997d6d329da38f866c0508742cc7761ea Mon Sep 17 00:00:00 2001 From: Vlad Temian Date: Fri, 31 Jul 2026 11:25:47 +0300 Subject: [PATCH 6/6] fix: close the runtime and coverage gaps found in review Externalise ws. Under --target node the bundler inlined the real npm package, and real ws never completes a handshake against Bun's node:http, so octto's socket hung in CONNECTING for every CLI user. Loading the bundle was not enough to catch that; the handshake now runs on both runtimes. Register the server error handler on the WebSocketServer. ws attaches its own listener to the HTTP server and re-emits on itself, so a handler on the HTTP server is registered too late to ever run: a bind failure crashed Node outright and left Bun waiting on a promise that never settled. Report a signal death as 128 + signum instead of collapsing it to 0. Both callers branch on exitCode, so a killed command read as a clean empty run. Reach schema.sql through import.meta.dirname; import.meta.path is Bun-only and left this file half-ported. Guard closeAllConnections, absent before Node 18.2, which would strand stop(). Pin the frame limit to Bun's 16 MB rather than inheriting ws's 100 MB. Narrow the server's dependency to SocketRouter, which removes the double cast from its tests. The tests earn their keep now. The timeout guard used a command that execs, so no grandchild held the pipes and reverting either fix still passed. The bundle test greps text it hardcodes rather than reading the build script, and never ran the artifact; it now loads it under both runtimes. loadSqlite's failure and the no-cache-on-failure path, the point of the whole change, had no coverage at all. Capturing logs had become blanket suppression, hiding stray output from every test that did not assert; unread lines now fail the test. --- package.json | 2 +- src/octto/session/server.ts | 46 +++-- src/octto/session/types.ts | 10 + src/tools/artifact-index/index.ts | 5 +- src/utils/config.ts | 6 + src/utils/process.ts | 35 +++- tests/config-loader.test.ts | 2 + tests/helpers/log-capture.ts | 30 ++- tests/hooks/constraint-reviewer.test.ts | 2 + .../integration/bundle-runtime-compat.test.ts | 102 ++++++++-- tests/mindmodel/loader.test.ts | 2 + tests/octto/server.test.ts | 174 +++++++++++------- tests/tools/artifact-index-runtime.test.ts | 109 +++++++++++ tests/utils/process.test.ts | 41 ++++- 14 files changed, 453 insertions(+), 113 deletions(-) create mode 100644 tests/tools/artifact-index-runtime.test.ts diff --git a/package.json b/package.json index 86484acf..01052191 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ ], "scripts": { "prepare": "lefthook install", - "build": "bun build src/index.ts --outdir dist --target node --external bun-pty --external jsonc-parser", + "build": "bun build src/index.ts --outdir dist --target node --external bun-pty --external jsonc-parser --external ws", "typecheck": "tsc --noEmit", "prepublishOnly": "bun run check && bun run build", "test": "bun test", diff --git a/src/octto/session/server.ts b/src/octto/session/server.ts index 3404514e..15e15b8e 100644 --- a/src/octto/session/server.ts +++ b/src/octto/session/server.ts @@ -8,8 +8,7 @@ import { config } from "@/utils/config"; import { extractErrorMessage } from "@/utils/errors"; import { log } from "@/utils/logger"; import { WsClientMessageSchema } from "./schemas"; -import type { SessionStore } from "./sessions"; -import type { SessionServer, SessionSocket, WsClientMessage } from "./types"; +import type { SessionServer, SessionSocket, SocketRouter, WsClientMessage } from "./types"; const WS_PATH = "/ws"; const HTML_PATHS = new Set(["/", "/index.html"]); @@ -36,7 +35,7 @@ function sendError(socket: SessionSocket, error: string, details: string): void socket.send(JSON.stringify({ type: "error", error, details })); } -function handleWsMessage(socket: SessionSocket, sessionId: string, raw: string, store: SessionStore): void { +function handleWsMessage(socket: SessionSocket, sessionId: string, raw: string, store: SocketRouter): void { let parsed: unknown; try { parsed = JSON.parse(raw); @@ -56,7 +55,7 @@ function handleWsMessage(socket: SessionSocket, sessionId: string, raw: string, store.handleWsMessage(sessionId, result.output as WsClientMessage); } -function attachWebSockets(wss: WebSocketServer, sessionId: string, store: SessionStore): void { +function attachWebSockets(wss: WebSocketServer, sessionId: string, store: SocketRouter): void { wss.on("connection", (socket: WebSocket) => { store.handleWsConnect(sessionId, socket); @@ -72,16 +71,37 @@ function attachWebSockets(wss: WebSocketServer, sessionId: string, store: Sessio }); } -function listen(http: Server, hostname: string): Promise { +// ws attaches its own listener to the HTTP server and re-emits its errors on +// itself, so the only handler that sees a bind failure is one registered here. +// Without it an unhandled 'error' propagates out and takes the host process +// down with the session. +function listen(http: Server, wss: WebSocketServer, hostname: string): Promise { return new Promise((resolve, reject) => { - http.once("error", reject); + let settled = false; + const settle = (finish: () => void): void => { + if (settled) return; + settled = true; + finish(); + }; + + wss.on("error", (error) => { + log.error(LOG_MODULE, "WebSocket server error", error); + settle(() => { + reject(error); + }); + }); + http.listen(0, hostname, () => { const address = http.address(); if (address === null || typeof address === "string") { - reject(new Error(ERR_NO_PORT)); + settle(() => { + reject(new Error(ERR_NO_PORT)); + }); return; } - resolve(address.port); + settle(() => { + resolve(address.port); + }); }); }); } @@ -93,7 +113,9 @@ function stop(http: Server, wss: WebSocketServer): Promise { client.terminate(); } wss.close(() => { - http.closeAllConnections(); + // Absent before Node 18.2, which older Electron builds still ship. An + // unguarded call throws inside ws's close callback and strands stop(). + http.closeAllConnections?.(); http.close(() => { resolve(); }); @@ -103,7 +125,7 @@ function stop(http: Server, wss: WebSocketServer): Promise { export async function createServer( sessionId: string, - store: SessionStore, + store: SocketRouter, ): Promise<{ server: SessionServer; port: number }> { const htmlBundle = getHtmlBundle(); const hostname = config.octto.allowRemoteBind ? config.octto.bindAddress : LOOPBACK; @@ -111,10 +133,10 @@ export async function createServer( const http = createHttpServer((req, res) => { serveHttp(req, res, htmlBundle); }); - const wss = new WebSocketServer({ server: http, path: WS_PATH }); + const wss = new WebSocketServer({ server: http, path: WS_PATH, maxPayload: config.octto.maxFrameBytes }); attachWebSockets(wss, sessionId, store); - const port = await listen(http, hostname); + const port = await listen(http, wss, hostname); return { port, diff --git a/src/octto/session/types.ts b/src/octto/session/types.ts index f2792bea..4bc5c614 100644 --- a/src/octto/session/types.ts +++ b/src/octto/session/types.ts @@ -195,6 +195,16 @@ export interface SessionSocket { send: (data: string) => void; } +/** + * The slice of the session store the transport drives. Narrower than + * SessionStore so the server depends only on what it actually calls. + */ +export interface SocketRouter { + handleWsConnect: (sessionId: string, socket: SessionSocket) => void; + handleWsDisconnect: (sessionId: string) => void; + handleWsMessage: (sessionId: string, message: WsClientMessage) => void; +} + /** The HTTP server surface octto needs, independent of runtime. */ export interface SessionServer { readonly port: number; diff --git a/src/tools/artifact-index/index.ts b/src/tools/artifact-index/index.ts index 25e44d5f..fbf5f6ae 100644 --- a/src/tools/artifact-index/index.ts +++ b/src/tools/artifact-index/index.ts @@ -340,11 +340,14 @@ async function initializeDb(dbPath: string): Promise { const Sqlite = await loadSqlite(); const database = new Sqlite(dbPath); - const schemaPath = join(dirname(import.meta.path), "schema.sql"); + // import.meta.dirname is the portable spelling; import.meta.dir and .path are + // Bun-only and read as undefined under Node. + const schemaPath = join(import.meta.dirname, "schema.sql"); let schema: string; try { schema = readFileSync(schemaPath, "utf-8"); } catch { + // Bundled builds ship no sibling schema.sql; fall back to the inline copy. schema = getInlineSchema(); } database.exec(schema); diff --git a/src/utils/config.ts b/src/utils/config.ts index f9842880..077475a5 100644 --- a/src/utils/config.ts +++ b/src/utils/config.ts @@ -4,6 +4,10 @@ const BYTES_PER_KB = 1024; const LARGE_FILE_KB = 100; +// Matches Bun.serve's default so the Node-hosted server accepts the same +// frames the Bun-hosted one did. +const MAX_FRAME_MB = 16; +const MAX_FRAME_KB = MAX_FRAME_MB * BYTES_PER_KB; const MS_PER_SECOND = 1000; const SECONDS_PER_MINUTE = 60; const ANSWER_TIMEOUT_MINUTES = 5; @@ -132,6 +136,8 @@ export const config = { bindAddress: "127.0.0.1", /** Allow overriding bind address for remote access */ allowRemoteBind: false, + /** Largest accepted WebSocket frame (bytes) */ + maxFrameBytes: MAX_FRAME_KB * BYTES_PER_KB, }, /** diff --git a/src/utils/process.ts b/src/utils/process.ts index dbe21edd..eb0e72d5 100644 --- a/src/utils/process.ts +++ b/src/utils/process.ts @@ -5,16 +5,28 @@ import { spawn } from "node:child_process"; import { accessSync, constants } from "node:fs"; +import { constants as osConstants } from "node:os"; import { delimiter, join } from "node:path"; import type { Readable } from "node:stream"; const WINDOWS_DEFAULT_EXTENSIONS = ".COM;.EXE;.BAT;.CMD"; const TIMEOUT_SIGNAL = "SIGTERM"; +// Shells report a signal death as 128 + signal number; mirror that so callers +// branching on exitCode cannot read a killed command as a clean exit. +const SIGNAL_EXIT_BASE = 128; +const UNKNOWN_SIGNAL_EXIT = 1; + +export interface CommandOptions { + readonly timeoutMs?: number; +} + export interface CommandResult { readonly stdout: string; readonly stderr: string; readonly exitCode: number; + /** Set when the command was terminated by a signal rather than exiting. */ + readonly signal?: NodeJS.Signals; } function isExecutable(candidate: string): boolean { @@ -58,6 +70,14 @@ function collect(stream: Readable, append: (chunk: string) => void): void { stream.on("data", append); } +// A signal death arrives as code null. Collapsing that to 0 would let callers +// that branch on exitCode read a killed command as a successful empty run. +function exitCodeFor(code: number | null, signal: NodeJS.Signals | null): number { + if (code !== null) return code; + if (signal === null) return UNKNOWN_SIGNAL_EXIT; + return SIGNAL_EXIT_BASE + (osConstants.signals[signal] ?? UNKNOWN_SIGNAL_EXIT); +} + /** * Run a command to completion, collecting stdout and stderr. * @@ -68,13 +88,13 @@ function collect(stream: Readable, append: (chunk: string) => void): void { */ export function runCommand( command: string, - args: string[], - options: { timeoutMs?: number } = {}, + args: readonly string[], + options: CommandOptions = {}, ): Promise { const { timeoutMs } = options; return new Promise((resolve, reject) => { - const child = spawn(command, args, { stdio: ["ignore", "pipe", "pipe"] }); + const child = spawn(command, [...args], { stdio: ["ignore", "pipe", "pipe"] }); let stdout = ""; let stderr = ""; @@ -82,13 +102,16 @@ export function runCommand( // Settle from the timer rather than waiting for "close": a killed shell can // leave a grandchild holding the stdio pipes open, which delays that event - // for as long as the original command would have run. + // for as long as the original command would have run. Drop the pipes too, + // so nothing keeps appending to strings no caller can still reach. const timer = timeoutMs === undefined ? undefined : setTimeout(() => { timedOut = true; child.kill(TIMEOUT_SIGNAL); + child.stdout.destroy(); + child.stderr.destroy(); reject(new Error(`${command} timed out after ${timeoutMs}ms`)); }, timeoutMs); @@ -103,10 +126,10 @@ export function runCommand( clearTimeout(timer); reject(error); }); - child.on("close", (code) => { + child.on("close", (code, signal) => { clearTimeout(timer); if (timedOut) return; - resolve({ stdout, stderr, exitCode: code ?? 0 }); + resolve({ stdout, stderr, exitCode: exitCodeFor(code, signal), ...(signal ? { signal } : {}) }); }); }); } diff --git a/tests/config-loader.test.ts b/tests/config-loader.test.ts index c97be594..3df833dc 100644 --- a/tests/config-loader.test.ts +++ b/tests/config-loader.test.ts @@ -890,7 +890,9 @@ describe("validateAgentModels", () => { }); afterEach(() => { + const stray = logs.unread(); logs.restore(); + expect(stray).toEqual([]); }); function createProvider(id: string, modelIds: string[]): ProviderInfo { diff --git a/tests/helpers/log-capture.ts b/tests/helpers/log-capture.ts index b94cf730..cfd8fe56 100644 --- a/tests/helpers/log-capture.ts +++ b/tests/helpers/log-capture.ts @@ -7,11 +7,17 @@ import { spyOn } from "bun:test"; * Tests that intentionally drive production code down a logging path use this * to keep the reporter output pristine while still asserting on what was * logged. Multi-argument calls are joined with a single space. + * + * Reading a channel marks it inspected. `unread()` then reports anything the + * test silently swallowed, so capturing cannot quietly become suppression: + * pair it with an afterEach that fails on a non-empty result. */ export interface LogCapture { readonly info: string[]; readonly warn: string[]; readonly error: string[]; + /** Captured lines on channels the test never looked at. */ + unread: () => string[]; restore: () => void; } @@ -21,6 +27,12 @@ export function captureLogs(): LogCapture { const info: string[] = []; const warn: string[] = []; const error: string[] = []; + const inspected = new Set(); + + const channel = (name: string, lines: string[]): string[] => { + inspected.add(name); + return lines; + }; const spies = [ spyOn(console, "log").mockImplementation((...args: unknown[]) => { @@ -35,9 +47,21 @@ export function captureLogs(): LogCapture { ]; return { - info, - warn, - error, + get info() { + return channel("info", info); + }, + get warn() { + return channel("warn", warn); + }, + get error() { + return channel("error", error); + }, + unread: () => + [ + ...(inspected.has("info") ? [] : info), + ...(inspected.has("warn") ? [] : warn), + ...(inspected.has("error") ? [] : error), + ].filter(Boolean), restore: () => { for (const spy of spies) { spy.mockRestore(); diff --git a/tests/hooks/constraint-reviewer.test.ts b/tests/hooks/constraint-reviewer.test.ts index 4f231534..d6b970b4 100644 --- a/tests/hooks/constraint-reviewer.test.ts +++ b/tests/hooks/constraint-reviewer.test.ts @@ -16,7 +16,9 @@ describe("createConstraintReviewerHook", () => { }); afterEach(() => { + const stray = logs.unread(); logs.restore(); + expect(stray).toEqual([]); rmSync(testDir, { recursive: true, force: true }); }); diff --git a/tests/integration/bundle-runtime-compat.test.ts b/tests/integration/bundle-runtime-compat.test.ts index 97d1cb81..1ead21d4 100644 --- a/tests/integration/bundle-runtime-compat.test.ts +++ b/tests/integration/bundle-runtime-compat.test.ts @@ -1,34 +1,96 @@ // tests/integration/bundle-runtime-compat.test.ts -import { describe, expect, it } from "bun:test"; +import { afterAll, beforeAll, describe, expect, it } from "bun:test"; +import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { join } from "node:path"; -const ENTRY = join(import.meta.dir, "../../src/index.ts"); -const EXTERNAL = ["bun-pty", "jsonc-parser"]; +import { runCommand } from "../../src/utils/process"; + +const ROOT = join(import.meta.dirname, "../.."); +const ENTRY = join(ROOT, "src/index.ts"); +// Inside node_modules so Node resolves the externalised bare specifiers the +// same way a consumer's install would. +const OUT_DIR = join(ROOT, "node_modules/.cache/micode-bundle-check"); +const OUT_FILE = join(OUT_DIR, "index.js"); + const STATIC_BUN_IMPORT = /^\s*import[^;]*from\s*["']bun:[a-z]+["']/m; +// Bun-only members of import.meta. Node leaves both undefined, so any survivor +// throws at whatever point it is reached. +const BUN_ONLY_IMPORT_META = /import\.meta\.(require|path|dir)\b/; +const LOAD_TIMEOUT_MS = 60_000; -// Mirrors the shipped build in package.json. OpenCode Desktop runs on -// Electron, so the published bundle has to resolve and execute under Node. -async function bundle(): Promise { - const built = await Bun.build({ entrypoints: [ENTRY], target: "node", external: EXTERNAL }); - expect(built.success).toBe(true); - return await built.outputs[0].text(); +/** + * The flags the published bundle is actually built with, read from the build + * script so this test cannot drift away from what ships. + */ +function shippedBuildFlags(): { target: string; external: string[] } { + const manifest: unknown = JSON.parse(readFileSync(join(ROOT, "package.json"), "utf8")); + const script = (manifest as { scripts: Record }).scripts.build; + + const target = /--target\s+(\S+)/.exec(script)?.[1]; + if (target === undefined) throw new Error(`build script declares no --target: ${script}`); + + const external = [...script.matchAll(/--external\s+(\S+)/g)].map((match) => match[1]); + return { target, external }; } +let bundle = ""; + +beforeAll(async () => { + const { target, external } = shippedBuildFlags(); + const built = await Bun.build({ + entrypoints: [ENTRY], + target: target as "node" | "bun", + external, + }); + expect(built.success).toBe(true); + + bundle = await built.outputs[0].text(); + mkdirSync(OUT_DIR, { recursive: true }); + writeFileSync(OUT_FILE, bundle, "utf8"); +}); + +afterAll(() => { + rmSync(OUT_DIR, { recursive: true, force: true }); +}); + describe("bundle runtime compatibility", () => { - it("emits no static bun: import that non-Bun ESM loaders reject", async () => { - const output = await bundle(); - expect(output).not.toMatch(STATIC_BUN_IMPORT); + // OpenCode Desktop runs on Electron. A static bun: specifier is rejected + // while the loader resolves the graph, before any code runs. + it("emits no static bun: import", () => { + expect(bundle).not.toMatch(STATIC_BUN_IMPORT); + }); + + it("emits no Bun-only import.meta member", () => { + expect(bundle).not.toMatch(BUN_ONLY_IMPORT_META); }); - // import.meta.require exists only in Bun; Node leaves it undefined and the - // generated __require shim throws on first call. - it("emits no import.meta.require shim", async () => { - const output = await bundle(); - expect(output).not.toContain("import.meta.require"); + it("still reaches bun:sqlite through a deferred import", () => { + expect(bundle).toContain('import("bun:sqlite")'); }); - it("still reaches bun:sqlite through a deferred import", async () => { - const output = await bundle(); - expect(output).toContain('import("bun:sqlite")'); + // The greps above only catch failure modes we already know about. Actually + // loading the artifact under each runtime catches the ones we do not. + it("loads under node", async () => { + const { stdout, stderr, exitCode } = await runCommand( + "node", + ["-e", `import(${JSON.stringify(OUT_FILE)}).then(m => console.log(Object.keys(m).join(",")))`], + { timeoutMs: LOAD_TIMEOUT_MS }, + ); + + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(stdout).toContain("OpenCodeConfigPlugin"); + }); + + it("loads under bun", async () => { + const { stdout, stderr, exitCode } = await runCommand( + "bun", + ["-e", `import(${JSON.stringify(OUT_FILE)}).then(m => console.log(Object.keys(m).join(",")))`], + { timeoutMs: LOAD_TIMEOUT_MS }, + ); + + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(stdout).toContain("OpenCodeConfigPlugin"); }); }); diff --git a/tests/mindmodel/loader.test.ts b/tests/mindmodel/loader.test.ts index f214c16e..6fd0ea0d 100644 --- a/tests/mindmodel/loader.test.ts +++ b/tests/mindmodel/loader.test.ts @@ -17,7 +17,9 @@ describe("mindmodel loader", () => { }); afterEach(() => { + const stray = logs.unread(); logs.restore(); + expect(stray).toEqual([]); rmSync(testDir, { recursive: true, force: true }); }); diff --git a/tests/octto/server.test.ts b/tests/octto/server.test.ts index 438e12a6..bbabf603 100644 --- a/tests/octto/server.test.ts +++ b/tests/octto/server.test.ts @@ -1,36 +1,40 @@ // tests/octto/server.test.ts // Exercises the real HTTP + WebSocket server against real client connections. import { afterEach, describe, expect, it } from "bun:test"; +import { connect as netConnect } from "node:net"; import { createServer } from "../../src/octto/session/server"; -import type { SessionServer, SessionSocket, WsClientMessage } from "../../src/octto/session/types"; +import type { SessionServer, SessionSocket, SocketRouter, WsClientMessage } from "../../src/octto/session/types"; import { captureLogs, type LogCapture } from "../helpers/log-capture"; const SESSION_ID = "test-session"; +const POLL_TIMEOUT_MS = 2000; +const POLL_INTERVAL_MS = 10; +const HANDSHAKE_TIMEOUT_MS = 4000; interface Recorded { - connects: SessionSocket[]; + connects: { sessionId: string; socket: SessionSocket }[]; disconnects: string[]; - messages: WsClientMessage[]; + messages: { sessionId: string; message: WsClientMessage }[]; } -function createRecordingStore(): { store: Parameters[1]; recorded: Recorded } { +function createRecordingRouter(): { router: SocketRouter; recorded: Recorded } { const recorded: Recorded = { connects: [], disconnects: [], messages: [] }; - const store = { - handleWsConnect: (_sessionId: string, socket: SessionSocket) => { - recorded.connects.push(socket); + const router: SocketRouter = { + handleWsConnect: (sessionId, socket) => { + recorded.connects.push({ sessionId, socket }); }, - handleWsDisconnect: (sessionId: string) => { + handleWsDisconnect: (sessionId) => { recorded.disconnects.push(sessionId); }, - handleWsMessage: (_sessionId: string, message: WsClientMessage) => { - recorded.messages.push(message); + handleWsMessage: (sessionId, message) => { + recorded.messages.push({ sessionId, message }); }, }; - return { store: store as unknown as Parameters[1], recorded }; + return { router, recorded }; } -function waitFor(predicate: () => boolean, timeoutMs = 2000): Promise { +function waitFor(describeCondition: string, predicate: () => boolean): Promise { const started = Date.now(); return new Promise((resolve, reject) => { const tick = (): void => { @@ -38,11 +42,11 @@ function waitFor(predicate: () => boolean, timeoutMs = 2000): Promise { resolve(); return; } - if (Date.now() - started > timeoutMs) { - reject(new Error("condition not met in time")); + if (Date.now() - started > POLL_TIMEOUT_MS) { + reject(new Error(`timed out waiting for: ${describeCondition}`)); return; } - setTimeout(tick, 10); + setTimeout(tick, POLL_INTERVAL_MS); }; tick(); }); @@ -60,6 +64,26 @@ function connect(port: number): Promise { }); } +/** Raw upgrade request, so we can see the status line for non-/ws paths. */ +function rawUpgrade(port: number, path: string): Promise { + return new Promise((resolve) => { + const socket = netConnect(port, "127.0.0.1", () => { + socket.write( + `GET ${path} HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\nUpgrade: websocket\r\nConnection: Upgrade\r\n` + + `Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\nSec-WebSocket-Version: 13\r\n\r\n`, + ); + }); + socket.on("data", (chunk) => { + resolve(chunk.toString().split("\r\n")[0]); + socket.destroy(); + }); + setTimeout(() => { + resolve("*** no response ***"); + socket.destroy(); + }, HANDSHAKE_TIMEOUT_MS); + }); +} + describe("octto session server", () => { let server: SessionServer | undefined; let logs: LogCapture | undefined; @@ -71,54 +95,69 @@ describe("octto session server", () => { server = undefined; }); - it("serves the html bundle on / and 404s unknown paths", async () => { - const { store } = createRecordingStore(); - const started = await createServer(SESSION_ID, store); + async function start(): Promise<{ port: number; recorded: Recorded }> { + const { router, recorded } = createRecordingRouter(); + const started = await createServer(SESSION_ID, router); server = started.server; + return { port: started.port, recorded }; + } + + it("serves the html bundle on / and on /index.html", async () => { + const { port } = await start(); + + for (const path of ["/", "/index.html", "/?theme=dark"]) { + const page = await fetch(`http://127.0.0.1:${port}${path}`); + expect(page.status).toBe(200); + expect(page.headers.get("content-type")).toContain("text/html"); + expect((await page.text()).length).toBeGreaterThan(0); + } + }); - const page = await fetch(`http://127.0.0.1:${started.port}/`); - expect(page.status).toBe(200); - expect(page.headers.get("content-type")).toContain("text/html"); - expect((await page.text()).length).toBeGreaterThan(0); - - const missing = await fetch(`http://127.0.0.1:${started.port}/nope`); + it("404s an unknown path", async () => { + const { port } = await start(); + const missing = await fetch(`http://127.0.0.1:${port}/nope`); expect(missing.status).toBe(404); }); - it("reports a real bound port", async () => { - const { store } = createRecordingStore(); - const started = await createServer(SESSION_ID, store); - server = started.server; + it("reports the bound port and host it actually listened on", async () => { + const { port } = await start(); - expect(started.port).toBeGreaterThan(0); - expect(started.server.hostname).toBe("127.0.0.1"); + expect(server?.hostname).toBe("127.0.0.1"); + expect(server?.port).toBe(port); + const page = await fetch(`http://127.0.0.1:${port}/`); + expect(page.status).toBe(200); }); - it("routes a websocket connect, message and disconnect to the store", async () => { - const { store, recorded } = createRecordingStore(); - const started = await createServer(SESSION_ID, store); - server = started.server; + it("refuses a websocket upgrade outside /ws", async () => { + const { port } = await start(); - const client = await connect(started.port); - await waitFor(() => recorded.connects.length === 1); + expect(await rawUpgrade(port, "/ws")).toContain("101"); + expect(await rawUpgrade(port, "/other")).toContain("400"); + }); + + it("routes a websocket connect, message and disconnect to the router", async () => { + const { port, recorded } = await start(); + + const client = await connect(port); + await waitFor("connect recorded", () => recorded.connects.length === 1); + expect(recorded.connects[0].sessionId).toBe(SESSION_ID); client.send(JSON.stringify({ type: "response", id: "q1", answer: { value: "yes" } })); - await waitFor(() => recorded.messages.length === 1); - expect(recorded.messages[0]).toMatchObject({ type: "response", id: "q1" }); + await waitFor("message recorded", () => recorded.messages.length === 1); + expect(recorded.messages[0].sessionId).toBe(SESSION_ID); + expect(recorded.messages[0].message).toMatchObject({ type: "response", id: "q1" }); client.close(); - await waitFor(() => recorded.disconnects.length === 1); + await waitFor("disconnect recorded", () => recorded.disconnects.length === 1); expect(recorded.disconnects[0]).toBe(SESSION_ID); }); - it("answers malformed json with an error frame instead of dropping the socket", async () => { + it("answers malformed json with an error frame and logs it", async () => { logs = captureLogs(); - const { store, recorded } = createRecordingStore(); - const started = await createServer(SESSION_ID, store); - server = started.server; + const { port, recorded } = await start(); - const client = await connect(started.port); - await waitFor(() => recorded.connects.length === 1); + const client = await connect(port); + await waitFor("connect recorded", () => recorded.connects.length === 1); const received: string[] = []; client.addEventListener("message", (event: MessageEvent) => { @@ -126,23 +165,22 @@ describe("octto session server", () => { }); client.send("not json at all"); - await waitFor(() => received.length === 1); + await waitFor("error frame received", () => received.length === 1); expect(JSON.parse(received[0])).toMatchObject({ type: "error", error: "Invalid message format" }); expect(recorded.messages).toHaveLength(0); - expect(logs.error.some((line) => line.includes("Failed to parse WebSocket message"))).toBe(true); + expect(logs.error).toHaveLength(1); + expect(logs.error[0]).toContain("[octto] Failed to parse WebSocket message"); client.close(); }); - it("rejects a well-formed message that fails schema validation", async () => { + it("rejects a well-formed message that fails schema validation and logs it", async () => { logs = captureLogs(); - const { store, recorded } = createRecordingStore(); - const started = await createServer(SESSION_ID, store); - server = started.server; + const { port, recorded } = await start(); - const client = await connect(started.port); - await waitFor(() => recorded.connects.length === 1); + const client = await connect(port); + await waitFor("connect recorded", () => recorded.connects.length === 1); const received: string[] = []; client.addEventListener("message", (event: MessageEvent) => { @@ -150,25 +188,37 @@ describe("octto session server", () => { }); client.send(JSON.stringify({ type: "definitely-not-a-real-message" })); - await waitFor(() => received.length === 1); + await waitFor("error frame received", () => received.length === 1); - expect(JSON.parse(received[0])).toMatchObject({ type: "error", error: "Invalid message schema" }); + const frame = JSON.parse(received[0]); + expect(frame).toMatchObject({ type: "error", error: "Invalid message schema" }); + expect(frame.details.length).toBeGreaterThan(0); expect(recorded.messages).toHaveLength(0); + expect(logs.error).toHaveLength(1); + expect(logs.error[0]).toContain("[octto] Invalid WebSocket message schema"); client.close(); }); it("stops cleanly while a client is still connected", async () => { - const { store, recorded } = createRecordingStore(); - const started = await createServer(SESSION_ID, store); + const { port, recorded } = await start(); - const client = await connect(started.port); - await waitFor(() => recorded.connects.length === 1); + const client = await connect(port); + await waitFor("connect recorded", () => recorded.connects.length === 1); - await started.server.stop(); - server = undefined; + // Deliberately not clearing `server` until stop() has resolved, so a + // failure here still leaves afterEach able to release the port. + await server?.stop(); + await expect(fetch(`http://127.0.0.1:${port}/`)).rejects.toThrow(); - await expect(fetch(`http://127.0.0.1:${started.port}/`)).rejects.toThrow(); client.close(); }); + + it("tolerates stop() being called more than once", async () => { + const { port } = await start(); + expect((await fetch(`http://127.0.0.1:${port}/`)).status).toBe(200); + + await server?.stop(); + await server?.stop(); + }); }); diff --git a/tests/tools/artifact-index-runtime.test.ts b/tests/tools/artifact-index-runtime.test.ts new file mode 100644 index 00000000..c6e49965 --- /dev/null +++ b/tests/tools/artifact-index-runtime.test.ts @@ -0,0 +1,109 @@ +// tests/tools/artifact-index-runtime.test.ts +// The artifact index needs Bun's sqlite. Under Node it must fail with a +// diagnosable error rather than taking the plugin down, which is the whole +// point of loading bun:sqlite lazily. +import { afterAll, describe, expect, it } from "bun:test"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { createArtifactIndex } from "../../src/tools/artifact-index"; +import { runCommand } from "../../src/utils/process"; + +const ROOT = join(import.meta.dirname, "../.."); +const OUT_DIR = join(ROOT, "node_modules/.cache/micode-artifact-index-check"); +const PROBE_SOURCE = join(OUT_DIR, "probe.ts"); +const PROBE_BUNDLE = join(OUT_DIR, "probe.js"); +const NODE_TIMEOUT_MS = 60_000; + +async function buildNodeProbe(): Promise { + mkdirSync(OUT_DIR, { recursive: true }); + const modulePath = JSON.stringify(join(ROOT, "src/tools/artifact-index")); + writeFileSync( + PROBE_SOURCE, + `import { createArtifactIndex, getArtifactIndex } from ${modulePath}; + export async function probe(dir) { + try { + await createArtifactIndex(dir).initialize(); + return "UNEXPECTED_SUCCESS"; + } catch (error) { + return error.message; + } + } + // Two calls in a row: the second must report the same real cause, not a + // downstream "not initialized" from a cached half-built index. + export async function probeRepeat() { + const messages = []; + for (let i = 0; i < 2; i++) { + try { + const index = await getArtifactIndex(); + await index.search("x"); + messages.push("UNEXPECTED_SUCCESS"); + } catch (error) { + messages.push(error.message); + } + } + return messages.join(" || "); + }`, + "utf8", + ); + + const built = await Bun.build({ entrypoints: [PROBE_SOURCE], target: "node", outdir: OUT_DIR }); + expect(built.success).toBe(true); +} + +describe("artifact index runtime requirements", () => { + afterAll(() => { + rmSync(OUT_DIR, { recursive: true, force: true }); + }); + + it("initializes normally under bun", async () => { + const dir = mkdtempSync(join(tmpdir(), "artifact-index-bun-")); + const index = createArtifactIndex(dir); + try { + await index.initialize(); + await index.indexPlan({ id: "p1", filePath: join(dir, "plan.md"), title: "Plan" }); + expect(await index.search("Plan")).toHaveLength(1); + } finally { + await index.close(); + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("fails under node with an error naming the missing capability", async () => { + await buildNodeProbe(); + const dir = mkdtempSync(join(tmpdir(), "artifact-index-node-")); + + try { + const { stdout, exitCode } = await runCommand( + "node", + ["-e", `import(${JSON.stringify(PROBE_BUNDLE)}).then(m => m.probe(${JSON.stringify(dir)})).then(console.log)`], + { timeoutMs: NODE_TIMEOUT_MS }, + ); + + expect(exitCode).toBe(0); + expect(stdout).not.toContain("UNEXPECTED_SUCCESS"); + // Must name the capability and the consequence, not leak a raw loader error. + expect(stdout).toContain("Artifact index requires Bun's sqlite"); + expect(stdout).toContain("will not be indexed or searchable"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("keeps reporting the real cause when initialization keeps failing", async () => { + await buildNodeProbe(); + + const { stdout, exitCode } = await runCommand( + "node", + ["-e", `import(${JSON.stringify(PROBE_BUNDLE)}).then(m => m.probeRepeat()).then(console.log)`], + { timeoutMs: NODE_TIMEOUT_MS }, + ); + + expect(exitCode).toBe(0); + const [first, second] = stdout.trim().split(" || "); + expect(first).toContain("Artifact index requires Bun's sqlite"); + // Caching a failed index would make this one "Database not initialized". + expect(second).toContain("Artifact index requires Bun's sqlite"); + }); +}); diff --git a/tests/utils/process.test.ts b/tests/utils/process.test.ts index fd6f4dba..8de05856 100644 --- a/tests/utils/process.test.ts +++ b/tests/utils/process.test.ts @@ -6,17 +6,33 @@ import { delimiter, join } from "node:path"; import { findExecutable, runCommand } from "../../src/utils/process"; +// Generous next to the 50ms timeouts under test, but far below the 30s sleeps +// a stalled rejection would wait for. +const SETTLE_BUDGET_MS = 2000; + describe("findExecutable", () => { let binDir: string; let originalPath: string | undefined; + let scratchDirs: string[]; + + const makeDir = (prefix: string): string => { + const dir = mkdtempSync(join(tmpdir(), prefix)); + scratchDirs.push(dir); + return dir; + }; beforeEach(() => { - binDir = mkdtempSync(join(tmpdir(), "process-util-test-")); + scratchDirs = []; + binDir = makeDir("process-util-test-"); originalPath = process.env.PATH; }); + // Cleanup runs here rather than inline, so a failing assertion cannot leak + // a temp directory. afterEach(() => { - rmSync(binDir, { recursive: true, force: true }); + for (const dir of scratchDirs) { + rmSync(dir, { recursive: true, force: true }); + } if (originalPath === undefined) { delete process.env.PATH; } else { @@ -48,14 +64,13 @@ describe("findExecutable", () => { }); it("searches every PATH entry in order", () => { - const second = mkdtempSync(join(tmpdir(), "process-util-second-")); + const second = makeDir("process-util-second-"); const binary = join(second, "later-tool"); writeFileSync(binary, "#!/bin/sh\nexit 0\n"); chmodSync(binary, 0o755); process.env.PATH = [binDir, second].join(delimiter); expect(findExecutable("later-tool")).toBe(binary); - rmSync(second, { recursive: true, force: true }); }); it("returns null when PATH is unset", () => { @@ -81,12 +96,22 @@ describe("runCommand", () => { await expect(runCommand("definitely-not-installed", [])).rejects.toThrow(); }); + // `sh -c "sleep 30"` is not good enough here: the shell execs into sleep, so + // there is no grandchild and the pipes close the instant it is signalled. + // Backgrounding keeps the shell alive with a child holding stdout and stderr, + // which is the shape that must not stall the rejection. it("rejects as soon as the timeout elapses, not when the command would end", async () => { const started = Date.now(); - await expect(runCommand("sh", ["-c", "sleep 30"], { timeoutMs: 50 })).rejects.toThrow(/timed out after 50ms/); - // Guards the regression where a grandchild held the pipes open and the - // rejection waited for the full sleep. - expect(Date.now() - started).toBeLessThan(2000); + await expect(runCommand("sh", ["-c", "sleep 30 & wait"], { timeoutMs: 50 })).rejects.toThrow( + /timed out after 50ms/, + ); + expect(Date.now() - started).toBeLessThan(SETTLE_BUDGET_MS); + }); + + it("reports a signal death as a non-zero exit rather than success", async () => { + const result = await runCommand("sh", ["-c", "kill -TERM $$"]); + expect(result.exitCode).not.toBe(0); + expect(result.signal).toBe("SIGTERM"); }); it("does not apply a timeout when none is given", async () => {