From 309855a06361f85c9855aecba7fe9b2d0950d248 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Tue, 19 May 2026 12:23:55 -0400 Subject: [PATCH 1/2] refactor(test/cli): migrate CLI harness short-lived path to Effect services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the tmpdir + short-lived spawn machinery in withCliFixture: - `os.tmpdir + fs.mkdir + addFinalizer` → `FileSystem.makeTempDirectoryScoped`. Cleanup is now scope-driven without a hand-rolled finalizer. - `Process.run` (legacy cross-spawn wrapper) → `AppProcess.run`, which is the same service already used by `git`, `ripgrep`, `format`, `mcp`, etc. One spawn primitive across src + tests. `stdin: "ignore"` is set explicitly because `ChildProcess.make` defaults stdin to "pipe", which caused `opencode run` to block on `Bun.stdin.text()` waiting for an EOF that the test never sent. The old wrapper defaulted to ignore, so this preserves the prior contract. `AppFileSystem.defaultLayer` + `AppProcess.defaultLayer` are provided internally so callers' signatures don't change. The help-snapshot path regex widens from `[a-z0-9]+` to `[A-Za-z0-9]+` because Node's `mkdtemp` suffix is mixed-case (unlike the prior `Math.random().toString(36)` suffix). Slice 1 of 2. Slice 2 will migrate the long-lived `serve` / `acp` builders, which use raw `Bun.spawn` today. --- .../test/cli/help/help-snapshots.test.ts | 5 +- packages/opencode/test/lib/cli-process.ts | 67 ++++++++++++++----- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/packages/opencode/test/cli/help/help-snapshots.test.ts b/packages/opencode/test/cli/help/help-snapshots.test.ts index 94ea803b2655..dd49b2b33a3c 100644 --- a/packages/opencode/test/cli/help/help-snapshots.test.ts +++ b/packages/opencode/test/cli/help/help-snapshots.test.ts @@ -29,7 +29,10 @@ import { normalizeForSnapshot, PATH_SEP } from "../../lib/snapshot" function normalize(text: string): string { return normalizeForSnapshot(text, { pathReplacements: [ - [new RegExp(`${PATH_SEP}oc-cli-[a-z0-9]+`, "g"), ""], + // Mixed-case [A-Za-z0-9] because node's mkdtemp suffix is mixed-case + // (the harness now uses FileSystem.makeTempDirectoryScoped under the + // hood). A `[a-z0-9]+` regex would leave uppercase chars trailing. + [new RegExp(`${PATH_SEP}oc-cli-[A-Za-z0-9]+`, "g"), ""], [/\s+\[string\] \[default: ""\]/g, ' [string] [default: ""]'], ], }) diff --git a/packages/opencode/test/lib/cli-process.ts b/packages/opencode/test/lib/cli-process.ts index 1f11bb4e794b..7e11482aee7c 100644 --- a/packages/opencode/test/lib/cli-process.ts +++ b/packages/opencode/test/lib/cli-process.ts @@ -18,12 +18,12 @@ // without changing the fixture. Long-lived commands like `serve` will need a // different return shape — see the TODO at the bottom of OpencodeCli. import type { TestOptions } from "bun:test" +import { AppFileSystem } from "@opencode-ai/core/filesystem" +import { AppProcess } from "@opencode-ai/core/process" import { Deferred, Duration, Effect, Layer, Queue, Scope, Stream } from "effect" import { FetchHttpClient, HttpClient } from "effect/unstable/http" +import { ChildProcess } from "effect/unstable/process" import path from "node:path" -import fs from "node:fs/promises" -import os from "node:os" -import { Process } from "@/util/process" import { TestLLMServer } from "./llm-server" import { testProviderConfig } from "./test-provider" import { it } from "./effect" @@ -182,28 +182,54 @@ export function withCliFixture( ): Effect.Effect { return Effect.gen(function* () { const llm = yield* TestLLMServer + const fs = yield* AppFileSystem.Service + const appProc = yield* AppProcess.Service - const home = path.join(os.tmpdir(), "oc-cli-" + Math.random().toString(36).slice(2)) - yield* Effect.promise(() => fs.mkdir(home, { recursive: true })) - yield* Effect.addFinalizer(() => - Effect.promise(() => fs.rm(home, { recursive: true, force: true }).catch(() => undefined)), - ) + // FileSystem.makeTempDirectoryScoped handles both creation and scope-tied + // cleanup — replaces the old mkdir + addFinalizer pair. + const home = yield* fs.makeTempDirectoryScoped({ prefix: "oc-cli-" }) const configJson = JSON.stringify(testProviderConfig(llm.url)) const env = isolatedEnv(home, configJson) const spawn = (args: string[], opts?: SpawnOpts): Effect.Effect => - Effect.promise(async () => { + Effect.gen(function* () { const start = Date.now() - // Process.run pipes stdout/stderr by default and returns them as Buffers. - const result = await Process.run(["bun", "run", "--conditions=browser", cliEntry, ...args], { + const timeoutMs = opts?.timeoutMs ?? 30_000 + // stdin: "ignore" so the child doesn't see a piped stdin and block + // on `Bun.stdin.text()` (see src/cli/cmd/run.ts — non-TTY stdin is + // consumed as the prompt). The old Process.run wrapper defaulted to + // ignore; ChildProcess.make defaults to pipe, so we set it explicitly. + const command = ChildProcess.make("bun", ["run", "--conditions=browser", cliEntry, ...args], { cwd: home, - timeout: opts?.timeoutMs ?? 30_000, - env: { ...process.env, ...env, ...opts?.env }, - nothrow: true, + env: { ...env, ...opts?.env }, + extendEnv: true, + stdin: "ignore", }) + // appProc.run collects stdout/stderr as Buffers and returns the exit + // code without throwing on non-zero — same contract the old + // `Process.run({ nothrow: true })` call had. A timeout surfaces as an + // AppProcessError, which we convert to a synthetic non-zero result so + // the test sees it through the normal `expectExit` path. + const result = yield* appProc.run(command).pipe( + Effect.timeoutOrElse({ + duration: Duration.millis(timeoutMs), + orElse: () => + Effect.succeed({ + command: "", + exitCode: -1, + stdout: Buffer.alloc(0), + stderr: Buffer.from(`TIMED OUT after ${timeoutMs}ms\n`), + stdoutTruncated: false, + stderrTruncated: false, + }), + }), + // AppProcessError without timeout/signal/stdin opts only fires on + // spawn failure (binary missing, etc.) — treat as defect. + Effect.orDie, + ) return { - exitCode: result.code, + exitCode: result.exitCode, stdout: result.stdout.toString(), stderr: result.stderr.toString(), durationMs: Date.now() - start, @@ -380,7 +406,16 @@ export function withCliFixture( return yield* fn({ llm, home, opencode }) // FetchHttpClient is provided so test bodies can `yield* HttpClient.HttpClient` // and hit endpoints on `opencode.serve()` without rolling their own fetch. - }).pipe(Effect.provide(Layer.mergeAll(TestLLMServer.layer, FetchHttpClient.layer))) + }).pipe( + Effect.provide( + Layer.mergeAll( + TestLLMServer.layer, + FetchHttpClient.layer, + AppFileSystem.defaultLayer, + AppProcess.defaultLayer, + ), + ), + ) } function parseJsonEvents(stdout: string): Array> { From d137a50822b7e717160ce7bd72b7a5f61e1a101a Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Tue, 19 May 2026 12:31:48 -0400 Subject: [PATCH 2/2] refactor(test/cli): use AppProcess.run built-in timeout, name spawn span MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two simplify-pass refinements on top of the harness migration: 1. Pass `timeout` into `AppProcess.run` instead of wrapping with an external `Effect.timeoutOrElse`. AppProcess.run is itself scoped, so the built-in timeout triggers the acquireRelease kill finalizer *before* surfacing the error — guaranteeing the child is dead by the time the test continues. The external wrapper variant raced the scope close and could leak the child past the test boundary. 2. Replace `Effect.orDie` with `Effect.catchTag("AppProcessError", ...)` so timeouts AND spawn failures both turn into a synthetic non-zero result with the error's command / stderr / cause as diagnostics — instead of crashing as a defect. The synthetic result is `satisfies AppProcess.RunResult` so it stays in sync with process.ts. 3. Name the spawn Effect span `"opencode.spawn"` via `Effect.fn` to match the existing `Effect.fn("opencode.serve")` / `.acp` siblings. --- packages/opencode/test/lib/cli-process.ts | 84 +++++++++++------------ 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/packages/opencode/test/lib/cli-process.ts b/packages/opencode/test/lib/cli-process.ts index 7e11482aee7c..7adaba37dddd 100644 --- a/packages/opencode/test/lib/cli-process.ts +++ b/packages/opencode/test/lib/cli-process.ts @@ -192,49 +192,49 @@ export function withCliFixture( const configJson = JSON.stringify(testProviderConfig(llm.url)) const env = isolatedEnv(home, configJson) - const spawn = (args: string[], opts?: SpawnOpts): Effect.Effect => - Effect.gen(function* () { - const start = Date.now() - const timeoutMs = opts?.timeoutMs ?? 30_000 - // stdin: "ignore" so the child doesn't see a piped stdin and block - // on `Bun.stdin.text()` (see src/cli/cmd/run.ts — non-TTY stdin is - // consumed as the prompt). The old Process.run wrapper defaulted to - // ignore; ChildProcess.make defaults to pipe, so we set it explicitly. - const command = ChildProcess.make("bun", ["run", "--conditions=browser", cliEntry, ...args], { - cwd: home, - env: { ...env, ...opts?.env }, - extendEnv: true, - stdin: "ignore", - }) - // appProc.run collects stdout/stderr as Buffers and returns the exit - // code without throwing on non-zero — same contract the old - // `Process.run({ nothrow: true })` call had. A timeout surfaces as an - // AppProcessError, which we convert to a synthetic non-zero result so - // the test sees it through the normal `expectExit` path. - const result = yield* appProc.run(command).pipe( - Effect.timeoutOrElse({ - duration: Duration.millis(timeoutMs), - orElse: () => - Effect.succeed({ - command: "", - exitCode: -1, - stdout: Buffer.alloc(0), - stderr: Buffer.from(`TIMED OUT after ${timeoutMs}ms\n`), - stdoutTruncated: false, - stderrTruncated: false, - }), - }), - // AppProcessError without timeout/signal/stdin opts only fires on - // spawn failure (binary missing, etc.) — treat as defect. - Effect.orDie, - ) - return { - exitCode: result.exitCode, - stdout: result.stdout.toString(), - stderr: result.stderr.toString(), - durationMs: Date.now() - start, - } + const spawn = Effect.fn("opencode.spawn")(function* (args: string[], opts?: SpawnOpts) { + const start = Date.now() + const timeoutMs = opts?.timeoutMs ?? 30_000 + // stdin: "ignore" so the child doesn't see a piped stdin and block + // on `Bun.stdin.text()` (see src/cli/cmd/run.ts — non-TTY stdin is + // consumed as the prompt). The old Process.run wrapper defaulted to + // ignore; ChildProcess.make defaults to pipe, so we set it explicitly. + const command = ChildProcess.make("bun", ["run", "--conditions=browser", cliEntry, ...args], { + cwd: home, + env: { ...env, ...opts?.env }, + extendEnv: true, + stdin: "ignore", }) + // Pass timeout to appProc.run rather than wrapping with + // Effect.timeoutOrElse externally: AppProcess.run is itself scoped, so + // its built-in timeout triggers the acquireRelease kill finalizer + // inside cross-spawn-spawner *before* surfacing the AppProcessError — + // guaranteeing the child is dead by the time the test continues. + // External timeoutOrElse interrupts the run fiber but races the + // scope close, which can leak the child past the test boundary. + // + // Catch AppProcessError (timeout OR spawn failure) and synthesize a + // non-zero result so the test sees it via the usual `expectExit` + // path rather than as an unhandled Effect failure. + const result = yield* appProc.run(command, { timeout: Duration.millis(timeoutMs) }).pipe( + Effect.catchTag("AppProcessError", (err) => + Effect.succeed({ + command: err.command, + exitCode: err.exitCode ?? -1, + stdout: Buffer.alloc(0), + stderr: Buffer.from((err.stderr ?? String(err.cause ?? err.message)) + "\n"), + stdoutTruncated: false, + stderrTruncated: false, + } satisfies AppProcess.RunResult), + ), + ) + return { + exitCode: result.exitCode, + stdout: result.stdout.toString(), + stderr: result.stderr.toString(), + durationMs: Date.now() - start, + } + }) const run = (message: string, opts?: RunOpts): Effect.Effect => { const argv: string[] = ["run"]