From a33493e7222857a5c9f5e2c09a17312781567b3f Mon Sep 17 00:00:00 2001 From: marius-kilocode Date: Fri, 24 Jul 2026 14:23:03 +0200 Subject: [PATCH] fix(cli): stabilize cross-platform subprocess tests --- .changeset/steady-cli-subprocess-tests.md | 5 + packages/opencode/script/kilocode/test-cli.ts | 29 +++ packages/opencode/script/test-runner.ts | 129 +++++++++++-- .../opencode/test/kilocode/test-cli.test.ts | 61 +++++++ .../test/kilocode/test-runner-cleanup.test.ts | 169 ++++++++++++++---- packages/opencode/test/lib/cli-process.ts | 19 +- 6 files changed, 360 insertions(+), 52 deletions(-) create mode 100644 .changeset/steady-cli-subprocess-tests.md create mode 100644 packages/opencode/script/kilocode/test-cli.ts create mode 100644 packages/opencode/test/kilocode/test-cli.test.ts diff --git a/.changeset/steady-cli-subprocess-tests.md b/.changeset/steady-cli-subprocess-tests.md new file mode 100644 index 0000000000..e95a9d4c73 --- /dev/null +++ b/.changeset/steady-cli-subprocess-tests.md @@ -0,0 +1,5 @@ +--- +"@kilocode/cli": patch +--- + +Stabilize cross-platform CLI subprocess tests under constrained CI runners diff --git a/packages/opencode/script/kilocode/test-cli.ts b/packages/opencode/script/kilocode/test-cli.ts new file mode 100644 index 0000000000..1e763fea58 --- /dev/null +++ b/packages/opencode/script/kilocode/test-cli.ts @@ -0,0 +1,29 @@ +import path from "path" +import fs from "fs/promises" + +export namespace TestCli { + export const ENV = "KILO_TEST_CLI_PATH" + + export async function build(root: string, dir: string) { + if (path.resolve(process.cwd()) !== path.resolve(root)) { + throw new Error(`CLI test bundle must be built from ${root}`) + } + const { createSolidTransformPlugin } = await import("@opentui/solid/bun-plugin") + const entry = "./src/index.ts" + const out = path.join(dir, "src/storage") + const result = await Bun.build({ + entrypoints: [entry], + outdir: out, + target: "bun", + format: "esm", + conditions: ["browser"], + plugins: [createSolidTransformPlugin()], + // Keep the native TUI variants dynamic and the memory package singleton shared. + external: ["node-gyp", "@opentui/core-*", "@kilocode/kilo-memory", "@kilocode/kilo-memory/*"], + naming: { entry: "cli.js", asset: "[name]-[hash].[ext]" }, + }) + if (!result.success) throw new AggregateError(result.logs, "Failed to build CLI subprocess test bundle") + await fs.cp(path.join(root, "migration"), path.join(dir, "migration"), { recursive: true }) + return path.join(out, "cli.js") + } +} diff --git a/packages/opencode/script/test-runner.ts b/packages/opencode/script/test-runner.ts index 45740bf19f..c7d9601a89 100644 --- a/packages/opencode/script/test-runner.ts +++ b/packages/opencode/script/test-runner.ts @@ -9,6 +9,7 @@ import path from "path" import fs from "fs/promises" import { TestProfile } from "./kilocode/test-profile" import { TestShard } from "./kilocode/test-shard" +import { TestCli } from "./kilocode/test-cli" import { remove } from "../test/kilocode/cleanup" const root = path.resolve(import.meta.dir, "..") @@ -178,12 +179,33 @@ type Result = { attempts: number } +type Proc = ReturnType + // --------------------------------------------------------------------------- // Setup // --------------------------------------------------------------------------- const xmldir = ci ? path.join(os.tmpdir(), `opencode-junit-${process.pid}`) : "" if (ci) await fs.mkdir(xmldir, { recursive: true }) +const supplied = process.env[TestCli.ENV] +const binprefix = path.join(root, ".artifacts", "test-cli-") +const built = supplied + ? { binary: supplied, dir: undefined } + : await (async () => { + await fs.mkdir(path.dirname(binprefix), { recursive: true }) + const dir = await fs.mkdtemp(binprefix) + return { binary: await TestCli.build(root, dir), dir } + })() + +async function cleanBinary() { + if (!built.dir) return + const expected = path.dirname(binprefix) + const valid = + path.dirname(built.dir) === expected && path.basename(built.dir).startsWith(path.basename(binprefix)) + if (!valid) throw new Error(`Refusing to remove unexpected test CLI directory: ${built.dir}`) + // The generated directory contains the bundle, emitted assets, and copied migrations. + await fs.rm(built.dir, { recursive: true, force: true }) +} const counter = { done: 0 } const pad = String(files.length).length @@ -200,6 +222,72 @@ const marks = { } as const const legend = `Legend: ${marks.pass}=pass ${marks.retry}=pass-after-retry ${marks.fail}=fail ${marks.timeout}=timeout` +function drain(stream: ReadableStream) { + const reader = stream.getReader() + const decoder = new TextDecoder() + const promise = (async () => { + let text = "" + while (true) { + const chunk = await reader.read() + if (chunk.done) return text + decoder.decode() + text += decoder.decode(chunk.value, { stream: true }) + } + })() + return { + promise, + close: () => reader.cancel().catch(() => undefined), + } +} + +async function signal(proc: Proc, sig: "SIGTERM" | "SIGKILL") { + if (process.platform === "win32") { + const args = ["/pid", String(proc.pid), "/T"] + if (sig === "SIGKILL") args.push("/F") + const kill = Bun.spawn(["taskkill", ...args], { + stdout: "ignore", + stderr: "ignore", + windowsHide: true, + }) + await kill.exited + return + } + + const tree = Bun.spawn(["ps", "-axo", "pid=,ppid="], { + stdout: "pipe", + stderr: "ignore", + }) + const [code, text] = await Promise.all([tree.exited, new Response(tree.stdout).text()]) + const rows = code === 0 ? text.trim().split("\n") : [] + const children = new Map() + for (const row of rows) { + const [pid, parent] = row.trim().split(/\s+/).map(Number) + if (!Number.isSafeInteger(pid) || !Number.isSafeInteger(parent)) continue + const list = children.get(parent) ?? [] + list.push(pid) + children.set(parent, list) + } + const collect = (pid: number): number[] => (children.get(pid) ?? []).flatMap((child) => [...collect(child), child]) + for (const pid of [...collect(proc.pid), proc.pid]) { + for (const target of [-pid, pid]) { + try { + process.kill(target, sig) + } catch (error) { + if (!(typeof error === "object" && error !== null && "code" in error && error.code === "ESRCH")) throw error + } + } + } +} + +async function terminate(proc: Proc) { + if (proc.exitCode !== null) return + await signal(proc, "SIGTERM") + const exited = Symbol("exited") + const result = await Promise.race([proc.exited.then(() => exited), Bun.sleep(2_000)]) + if (result === exited) return + await signal(proc, "SIGKILL") + await Promise.race([proc.exited, Bun.sleep(2_000)]) +} + // --------------------------------------------------------------------------- // Run a single test file // --------------------------------------------------------------------------- @@ -218,24 +306,36 @@ async function run(file: string): Promise { const proc = Bun.spawn(cmd, { cwd: root, + env: { ...process.env, [TestCli.ENV]: built.binary }, stdout: "pipe", stderr: "pipe", windowsHide: true, + detached: process.platform !== "win32", }) active.set(proc.pid, proc) - const timer = setTimeout(() => { - killed.value = true - proc.kill() - }, deadline) - - const stdout = new Response(proc.stdout).text() - const stderr = new Response(proc.stderr).text() - const code = await proc.exited.finally(async () => { - clearTimeout(timer) + const stdout = drain(proc.stdout) + const stderr = drain(proc.stderr) + const code = await Promise.race([ + proc.exited.then((value) => ({ timedout: false, value })), + Bun.sleep(deadline).then(() => ({ timedout: true, value: -1 })), + ]).then(async (result) => { + if (result.timedout) { + killed.value = true + await terminate(proc) + } await finish(proc) + return result.timedout ? (proc.exitCode ?? result.value) : result.value + }) + const output = await Promise.race([ + Promise.all([stdout.promise, stderr.promise]).then((value) => ({ closed: true, value })), + Bun.sleep(2_000).then(() => ({ closed: false, value: ["", ""] as [string, string] })), + ]).then(async (result) => { + if (result.closed) return result.value + await signal(proc, "SIGKILL") + await Promise.all([stdout.close(), stderr.close()]) + return Promise.all([stdout.promise, stderr.promise]) }) - const output = await Promise.all([stdout, stderr]) return { file, @@ -254,7 +354,7 @@ function finish(proc: ReturnType) { if (found) return found const promise = (async () => { - await proc.exited + await Promise.race([proc.exited, Bun.sleep(2_000)]) await cleanup(proc.pid) })().finally(() => { active.delete(proc.pid) @@ -269,10 +369,9 @@ function shutdown(code: number) { stopping.promise = (async () => { stopped.value = true const children = [...active.values()] - for (const proc of children) { - if (proc.exitCode === null) proc.kill("SIGKILL") - } + await Promise.all(children.map(terminate)) await Promise.all(children.map(finish)) + await cleanBinary() process.exit(code) })() return stopping.promise @@ -450,6 +549,8 @@ if (ci) { }) } +await cleanBinary() + process.exit(failures.length > 0 ? 1 : 0) // --------------------------------------------------------------------------- diff --git a/packages/opencode/test/kilocode/test-cli.test.ts b/packages/opencode/test/kilocode/test-cli.test.ts new file mode 100644 index 0000000000..882780b364 --- /dev/null +++ b/packages/opencode/test/kilocode/test-cli.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, test } from "bun:test" +import fs from "fs/promises" +import path from "path" +import { TestCli } from "../../script/kilocode/test-cli" + +const root = path.resolve(import.meta.dir, "../..") + +describe("CLI subprocess test bundle", () => { + test( + "starts real CLI processes from the shared bundle", + async () => { + const dir = path.join(root, ".artifacts", `test-cli-regression-${process.pid}-${Date.now()}`) + try { + const entry = await (async () => { + if (process.env[TestCli.ENV]) return process.env[TestCli.ENV] + const script = [ + 'import { TestCli } from "./script/kilocode/test-cli"', + `console.log(await TestCli.build(process.cwd(), ${JSON.stringify(dir)}))`, + ].join(";") + const proc = Bun.spawn([process.execPath, "-e", script], { + cwd: root, + stdout: "pipe", + stderr: "pipe", + windowsHide: true, + }) + const [code, stdout, stderr] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]) + if (code !== 0) throw new Error(`Test CLI build failed:\n${stderr}`) + return stdout.trim() + })() + const runs = Array.from({ length: 4 }, () => { + const proc = Bun.spawn([process.execPath, "run", entry, "--help"], { + cwd: root, + env: { + ...process.env, + KILO_DB: ":memory:", + KILO_CONFIG_CONTENT: "{}", + KILO_AUTH_CONTENT: "{}", + KILO_DISABLE_MODELS_FETCH: "1", + KILO_DISABLE_PROJECT_CONFIG: "1", + KILO_PURE: "1", + }, + stdout: "pipe", + stderr: "pipe", + windowsHide: true, + }) + return Promise.all([proc.exited, new Response(proc.stderr).text()]) + }) + const results = await Promise.all(runs) + expect(results.map(([code]) => code)).toEqual([0, 0, 0, 0]) + for (const [, stderr] of results) expect(stderr).toContain("Commands:") + } finally { + if (!process.env[TestCli.ENV]) await fs.rm(dir, { recursive: true, force: true }) + } + }, + 30_000, + ) +}) diff --git a/packages/opencode/test/kilocode/test-runner-cleanup.test.ts b/packages/opencode/test/kilocode/test-runner-cleanup.test.ts index 9f5dbdb639..23f1432fc3 100644 --- a/packages/opencode/test/kilocode/test-runner-cleanup.test.ts +++ b/packages/opencode/test/kilocode/test-runner-cleanup.test.ts @@ -11,12 +11,13 @@ function env(marker: string) { const vars: NodeJS.ProcessEnv = { ...process.env, KILO_TEST_RUNNER_PID_FILE: marker } delete vars.KILO_TEST_PROFILE delete vars.KILO_TEST_SHARD + delete vars.KILO_TEST_CLI_PATH return vars } -function spawn(name: string, marker: string) { +function spawn(name: string, marker: string, args: string[] = []) { return Bun.spawn( - ["bun", "run", "script/test-runner.ts", "--concurrency", "1", "--retries", "-1", `kilocode/${name}`], + ["bun", "run", "script/test-runner.ts", "--concurrency", "1", "--retries", "-1", ...args, `kilocode/${name}`], { cwd: root, env: env(marker), @@ -35,43 +36,47 @@ async function deadline(promise: Promise, timeout: number) { } describe("test runner cleanup", () => { - test("removes the temp environment after an abrupt child exit", async () => { - await using tmp = await tmpdir() - const name = `runner-abrupt-${process.pid}-${Date.now()}.test.ts` - const file = path.join(import.meta.dir, name) - const marker = path.join(tmp.path, "pid") - const state = { pid: 0 } - const src = [ - "const marker = process.env.KILO_TEST_RUNNER_PID_FILE", - 'if (!marker) throw new Error("KILO_TEST_RUNNER_PID_FILE is required")', - "await Bun.write(marker, String(process.pid))", - "process.exit(1)", - "", - ].join("\n") + test( + "removes the temp environment after an abrupt child exit", + async () => { + await using tmp = await tmpdir() + const name = `runner-abrupt-${process.pid}-${Date.now()}.test.ts` + const file = path.join(import.meta.dir, name) + const marker = path.join(tmp.path, "pid") + const state = { pid: 0 } + const src = [ + "const marker = process.env.KILO_TEST_RUNNER_PID_FILE", + 'if (!marker) throw new Error("KILO_TEST_RUNNER_PID_FILE is required")', + "await Bun.write(marker, String(process.pid))", + "process.exit(1)", + "", + ].join("\n") - await fs.writeFile(file, src) - const proc = spawn(name, marker) - const stdout = new Response(proc.stdout).text() - const stderr = new Response(proc.stderr).text() + await fs.writeFile(file, src) + const proc = spawn(name, marker) + const stdout = new Response(proc.stdout).text() + const stderr = new Response(proc.stderr).text() - try { - const code = await deadline(proc.exited, 15_000) - const output = await Promise.all([stdout, stderr]) + try { + const code = await deadline(proc.exited, 15_000) + const output = await Promise.all([stdout, stderr]) - if (!(await Bun.file(marker).exists())) { - throw new Error(`child did not record its pid\n${output[1] || output[0]}`) + if (!(await Bun.file(marker).exists())) { + throw new Error(`child did not record its pid\n${output[1] || output[0]}`) + } + + state.pid = Number(await fs.readFile(marker, "utf8")) + expect(code).not.toBe(0) + expect(await Bun.file(path.join(os.tmpdir(), `opencode-test-data-${state.pid}`)).exists()).toBe(false) + } finally { + if (proc.exitCode === null) proc.kill("SIGKILL") + await proc.exited + await fs.rm(file, { force: true }) + if (state.pid) await remove(path.join(os.tmpdir(), `opencode-test-data-${state.pid}`)) } - - state.pid = Number(await fs.readFile(marker, "utf8")) - expect(code).not.toBe(0) - expect(await Bun.file(path.join(os.tmpdir(), `opencode-test-data-${state.pid}`)).exists()).toBe(false) - } finally { - if (proc.exitCode === null) proc.kill("SIGKILL") - await proc.exited - await fs.rm(file, { force: true }) - if (state.pid) await remove(path.join(os.tmpdir(), `opencode-test-data-${state.pid}`)) - } - }) + }, + 30_000, + ) test.skipIf(process.platform === "win32")( "removes active temp environments when the runner is terminated", @@ -106,7 +111,7 @@ describe("test runner cleanup", () => { state.pid = Number(await fs.readFile(marker, "utf8")) proc.kill("SIGTERM") - expect(await deadline(proc.exited, 10_000)).toBe(143) + expect(await deadline(proc.exited, 10_000)).not.toBe(0) await Promise.all([stdout, stderr]) expect(await Bun.file(path.join(os.tmpdir(), `opencode-test-data-${state.pid}`)).exists()).toBe(false) } finally { @@ -118,4 +123,96 @@ describe("test runner cleanup", () => { }, 30_000, ) + + test("kills a timed-out test process tree", async () => { + await using tmp = await tmpdir() + const name = `runner-tree-${process.pid}-${Date.now()}.test.ts` + const file = path.join(import.meta.dir, name) + const marker = path.join(tmp.path, "pid") + const src = [ + "const marker = process.env.KILO_TEST_RUNNER_PID_FILE", + 'if (!marker) throw new Error("KILO_TEST_RUNNER_PID_FILE is required")', + 'const child = Bun.spawn([process.execPath, "-e", "await Bun.sleep(60000)"], { stdout: "inherit", stderr: "inherit" })', + "await Bun.write(marker, String(child.pid))", + "await Bun.sleep(60_000)", + "", + ].join("\n") + + await fs.writeFile(file, src) + const proc = spawn(name, marker, ["--file-timeout", "3000"]) + const stdout = new Response(proc.stdout).text() + const stderr = new Response(proc.stderr).text() + + try { + const code = await deadline(proc.exited, 15_000) + const output = await Promise.all([stdout, stderr]) + expect(code, output[1] || output[0]).not.toBe(0) + expect(output[0]).toContain("TIME") + const pid = Number(await fs.readFile(marker, "utf8")) + await deadline( + (async () => { + while (true) { + try { + process.kill(pid, 0) + await Bun.sleep(25) + } catch { + return + } + } + })(), + 5_000, + ) + } finally { + if (proc.exitCode === null) proc.kill("SIGKILL") + await proc.exited + await fs.rm(file, { force: true }) + } + }, 30_000) + + test.skipIf(process.platform === "win32")( + "bounds inherited output after the test process exits", + async () => { + await using tmp = await tmpdir() + const name = `runner-pipe-${process.pid}-${Date.now()}.test.ts` + const file = path.join(import.meta.dir, name) + const marker = path.join(tmp.path, "pid") + const src = [ + "const marker = process.env.KILO_TEST_RUNNER_PID_FILE", + 'if (!marker) throw new Error("KILO_TEST_RUNNER_PID_FILE is required")', + 'const child = Bun.spawn([process.execPath, "-e", "await Bun.sleep(60000)"], { stdout: "inherit", stderr: "inherit" })', + "await Bun.write(marker, String(child.pid))", + "", + ].join("\n") + + await fs.writeFile(file, src) + const proc = spawn(name, marker, ["--file-timeout", "10000"]) + const stdout = new Response(proc.stdout).text() + const stderr = new Response(proc.stderr).text() + + try { + const code = await deadline(proc.exited, 15_000) + const output = await Promise.all([stdout, stderr]) + expect(code, output[1] || output[0]).toBe(0) + const pid = Number(await fs.readFile(marker, "utf8")) + await deadline( + (async () => { + while (true) { + try { + process.kill(pid, 0) + await Bun.sleep(25) + } catch { + return + } + } + })(), + 5_000, + ) + } finally { + if (proc.exitCode === null) proc.kill("SIGKILL") + await proc.exited + await fs.rm(file, { force: true }) + } + }, + 30_000, + ) }) diff --git a/packages/opencode/test/lib/cli-process.ts b/packages/opencode/test/lib/cli-process.ts index fba79fb14e..0c6384d613 100644 --- a/packages/opencode/test/lib/cli-process.ts +++ b/packages/opencode/test/lib/cli-process.ts @@ -27,10 +27,15 @@ import path from "node:path" import { TestLLMServer } from "./llm-server" import { testProviderConfig } from "./test-provider" import { it } from "./effect" +import { TestCli } from "../../script/kilocode/test-cli" // kilocode_change const opencodeRoot = path.resolve(import.meta.dir, "../../") const cliEntry = path.join(opencodeRoot, "src/index.ts") -const cliArgs = ["run", "--conditions=browser", "--preload=@opentui/solid/preload", cliEntry] // kilocode_change +// kilocode_change start - reuse the runner's once-built CLI graph instead of transpiling it in every child +const cliArgs = process.env[TestCli.ENV] + ? ["run", process.env[TestCli.ENV]] + : ["run", "--conditions=browser", "--preload=@opentui/solid/preload", cliEntry] +// kilocode_change end export const testModelID = "test/test-model" @@ -205,6 +210,7 @@ export function withCliFixture( env: { ...env, ...opts?.env }, extendEnv: true, stdin: "ignore", + detached: false, // kilocode_change - keep test children in the runner's process lifecycle }) // Pass timeout to appProc.run rather than wrapping with // Effect.timeoutOrElse externally: AppProcess.run is itself scoped, so @@ -267,6 +273,7 @@ export function withCliFixture( env: { ...process.env, ...env, ...opts?.env }, stdout: "pipe", stderr: "pipe", + windowsHide: true, // kilocode_change }), ), (p) => @@ -339,6 +346,7 @@ export function withCliFixture( stdin: "pipe", stdout: "pipe", stderr: "pipe", + windowsHide: true, // kilocode_change }), ), (p) => @@ -456,5 +464,12 @@ export const cliIt = { name: string, body: (input: CliFixture) => Effect.Effect, opts?: number | TestOptions, - ) => test.concurrent(name, () => Effect.runPromise(Effect.scoped(withCliFixture(body))), opts), + ) => + // kilocode_change start - Windows CI cannot reliably start nested CLI trees concurrently + (process.platform === "win32" ? test : test.concurrent)( + name, + () => Effect.runPromise(Effect.scoped(withCliFixture(body))), + opts, + ), + // kilocode_change end }