From 80e5fb11c231455e7d282f2d3baeeec8d512beea Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Tue, 19 May 2026 16:18:19 -0400 Subject: [PATCH] refactor(test/cli): migrate harness short-lived path to AppProcess + FileSystem (#28366) --- .../test/cli/help/help-snapshots.test.ts | 5 +- packages/opencode/test/lib/cli-process.ts | 85 +++++++++++++------ 2 files changed, 64 insertions(+), 26 deletions(-) diff --git a/packages/opencode/test/cli/help/help-snapshots.test.ts b/packages/opencode/test/cli/help/help-snapshots.test.ts index 94ea803b26..dd49b2b33a 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 1f11bb4e79..7adaba37dd 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,33 +182,59 @@ 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 () => { - 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], { - cwd: home, - timeout: opts?.timeoutMs ?? 30_000, - env: { ...process.env, ...env, ...opts?.env }, - nothrow: true, - }) - return { - exitCode: result.code, - 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"] @@ -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> {