From 7f9eda62015f2056288a38cb433853c775297b41 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Fri, 27 Mar 2026 16:40:47 -0400 Subject: [PATCH 1/3] refactor(format): use ChildProcessSpawner instead of Process.spawn Replace raw Process.spawn with Effect's ChildProcessSpawner in the Format service. formatFile now returns an Effect instead of a Promise, composing natively with the layer. --- packages/opencode/specs/effect-migration.md | 83 ++++++++++++++++++-- packages/opencode/src/format/index.ts | 65 ++++++++------- packages/opencode/test/format/format.test.ts | 10 +-- 3 files changed, 117 insertions(+), 41 deletions(-) diff --git a/packages/opencode/specs/effect-migration.md b/packages/opencode/specs/effect-migration.md index f4acc6e52e..93b9cf8fb9 100644 --- a/packages/opencode/specs/effect-migration.md +++ b/packages/opencode/specs/effect-migration.md @@ -212,8 +212,81 @@ Fully migrated (single namespace, InstanceState where needed, flattened facade): Still open and likely worth migrating: -- [ ] `Session` -- [ ] `SessionProcessor` -- [ ] `SessionPrompt` -- [ ] `SessionCompaction` -- [ ] `Provider` +- [x] `Session` — `session/index.ts` +- [ ] `SessionProcessor` — blocked by AI SDK v6 PR (#18433) +- [ ] `SessionPrompt` — blocked by AI SDK v6 PR (#18433) +- [ ] `SessionCompaction` — blocked by AI SDK v6 PR (#18433) +- [ ] `Provider` — blocked by AI SDK v6 PR (#18433) + +Other services not yet migrated: + +- [ ] `SessionSummary` — `session/summary.ts` +- [ ] `SessionTodo` — `session/todo.ts` +- [ ] `SessionRevert` — `session/revert.ts` +- [ ] `Instruction` — `session/instruction.ts` +- [ ] `ShareNext` — `share/share-next.ts` +- [ ] `SyncEvent` — `sync/index.ts` +- [ ] `Storage` — `storage/storage.ts` +- [ ] `Workspace` — `control-plane/workspace.ts` + +## Tool interface → Effect + +Once individual tools are effectified, change `Tool.Info` (`tool/tool.ts`) so `init` and `execute` return `Effect` instead of `Promise`. This lets tool implementations compose natively with the Effect pipeline rather than being wrapped in `Effect.promise()` at the call site. Requires: + +1. Migrate each tool to return Effects +2. Update `Tool.define()` factory to work with Effects +3. Update `SessionPrompt` to `yield*` tool results instead of `await`ing — blocked by AI SDK v6 PR (#18433) + +Individual tools, ordered by value: + +- [ ] `apply_patch.ts` — HIGH: multi-step orchestration, error accumulation, Bus events +- [ ] `read.ts` — HIGH: streaming I/O, readline, binary detection → FileSystem + Stream +- [ ] `edit.ts` — HIGH: multi-step diff/format/publish pipeline, FileWatcher lock +- [ ] `grep.ts` — MEDIUM: spawns ripgrep → ChildProcessSpawner, timeout handling +- [ ] `write.ts` — MEDIUM: permission checks, diagnostics polling, Bus events +- [ ] `codesearch.ts` — MEDIUM: HTTP + SSE + manual timeout → HttpClient + Effect.timeout +- [ ] `webfetch.ts` — MEDIUM: fetch with UA retry, size limits → HttpClient +- [ ] `websearch.ts` — MEDIUM: MCP over HTTP → HttpClient +- [ ] `batch.ts` — MEDIUM: parallel execution, per-call error recovery → Effect.all +- [ ] `task.ts` — MEDIUM: task state management +- [ ] `glob.ts` — LOW: simple async generator +- [ ] `lsp.ts` — LOW: dispatch switch over LSP operations +- [ ] `skill.ts` — LOW: skill tool adapter +- [ ] `plan.ts` — LOW: plan file operations + +## Effect service adoption in already-migrated code + +Some services are effectified but still use raw `Filesystem.*` or `Process.spawn` instead of the Effect equivalents. These are low-hanging fruit — the layers already exist, they just need the dependency swap. + +### `Filesystem.*` → `AppFileSystem.Service` (yield in layer) + +- [ ] `file/index.ts` — 11 calls (the File service itself) +- [ ] `config/config.ts` — 7 calls +- [ ] `auth/index.ts` — 3 calls +- [ ] `skill/index.ts` — 3 calls +- [ ] `file/time.ts` — 1 call + +### `Process.spawn` → `ChildProcessSpawner` (yield in layer) + +- [ ] `format/index.ts` — 1 call + +## Filesystem consolidation + +`util/filesystem.ts` (raw fs wrapper) is used by **64 files**. The effectified `AppFileSystem` service (`filesystem/index.ts`) exists but only has **8 consumers**. As services and tools are effectified, they should switch from `Filesystem.*` to yielding `AppFileSystem.Service` — this happens naturally during each migration, not as a separate effort. + +Similarly, **28 files** still import raw `fs` or `fs/promises` directly. These should migrate to `AppFileSystem` or `Filesystem.*` as they're touched. + +Current raw fs users that will convert during tool migration: +- `tool/read.ts` — fs.createReadStream, readline +- `tool/apply_patch.ts` — fs/promises +- `tool/bash.ts` — fs/promises +- `file/ripgrep.ts` — fs/promises +- `storage/storage.ts` — fs/promises +- `patch/index.ts` — fs, fs/promises + +## Primitives & utilities + +- [ ] `util/lock.ts` — reader-writer lock → Effect Semaphore/Permit +- [ ] `util/flock.ts` — file-based distributed lock with heartbeat → Effect.repeat + addFinalizer +- [ ] `util/process.ts` — child process spawn wrapper → return Effect instead of Promise +- [ ] `util/lazy.ts` — replace uses in Effect code with Effect.cached; keep for sync-only code diff --git a/packages/opencode/src/format/index.ts b/packages/opencode/src/format/index.ts index 314e8c6e71..47b7d76b77 100644 --- a/packages/opencode/src/format/index.ts +++ b/packages/opencode/src/format/index.ts @@ -1,4 +1,6 @@ import { Effect, Layer, ServiceMap } from "effect" +import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process" +import * as CrossSpawnSpawner from "@/effect/cross-spawn-spawner" import { InstanceState } from "@/effect/instance-state" import { makeRuntime } from "@/effect/run-service" import path from "path" @@ -6,7 +8,6 @@ import { mergeDeep } from "remeda" import z from "zod" import { Config } from "../config/config" import { Instance } from "../project/instance" -import { Process } from "../util/process" import { Log } from "../util/log" import * as Formatter from "./formatter" @@ -36,6 +37,7 @@ export namespace Format { Service, Effect.gen(function* () { const config = yield* Config.Service + const spawner = yield* ChildProcessSpawner.ChildProcessSpawner const state = yield* InstanceState.make( Effect.fn("Format.state")(function* (_ctx) { @@ -98,38 +100,45 @@ export namespace Format { return checks.filter((x) => x.enabled).map((x) => x.item) } - async function formatFile(filepath: string) { - log.info("formatting", { file: filepath }) - const ext = path.extname(filepath) + function formatFile(filepath: string) { + return Effect.gen(function* () { + log.info("formatting", { file: filepath }) + const ext = path.extname(filepath) - for (const item of await getFormatter(ext)) { - log.info("running", { command: item.command }) - try { - const proc = Process.spawn( - item.command.map((x) => x.replace("$FILE", filepath)), - { - cwd: Instance.directory, - env: { ...process.env, ...item.environment }, - stdout: "ignore", - stderr: "ignore", - }, - ) - const exit = await proc.exited - if (exit !== 0) { + for (const item of yield* Effect.promise(() => getFormatter(ext))) { + log.info("running", { command: item.command }) + const cmd = item.command.map((x) => x.replace("$FILE", filepath)) + const code = yield* spawner + .spawn( + ChildProcess.make(cmd[0]!, cmd.slice(1), { + cwd: Instance.directory, + env: item.environment, + extendEnv: true, + }), + ) + .pipe( + Effect.flatMap((handle) => handle.exitCode), + Effect.scoped, + Effect.catch(() => + Effect.sync(() => { + log.error("failed to format file", { + error: "spawn failed", + command: item.command, + ...item.environment, + file: filepath, + }) + return ChildProcessSpawner.ExitCode(1) + }), + ), + ) + if (code !== 0) { log.error("failed", { command: item.command, ...item.environment, }) } - } catch (error) { - log.error("failed to format file", { - error, - command: item.command, - ...item.environment, - file: filepath, - }) } - } + }) } log.info("init") @@ -162,14 +171,14 @@ export namespace Format { const file = Effect.fn("Format.file")(function* (filepath: string) { const { formatFile } = yield* InstanceState.get(state) - yield* Effect.promise(() => formatFile(filepath)) + yield* formatFile(filepath) }) return Service.of({ init, status, file }) }), ) - export const defaultLayer = layer.pipe(Layer.provide(Config.defaultLayer)) + export const defaultLayer = layer.pipe(Layer.provide(Config.defaultLayer), Layer.provide(CrossSpawnSpawner.defaultLayer)) const { runPromise } = makeRuntime(Service, defaultLayer) diff --git a/packages/opencode/test/format/format.test.ts b/packages/opencode/test/format/format.test.ts index 6a9b4f5eda..cc7f55aaf1 100644 --- a/packages/opencode/test/format/format.test.ts +++ b/packages/opencode/test/format/format.test.ts @@ -1,17 +1,11 @@ -import { NodeChildProcessSpawner, NodeFileSystem, NodePath } from "@effect/platform-node" import { describe, expect } from "bun:test" -import { Effect, Layer } from "effect" +import { Effect } from "effect" import { provideTmpdirInstance } from "../fixture/fixture" import { testEffect } from "../lib/effect" import { Format } from "../../src/format" -import { Config } from "../../src/config/config" import * as Formatter from "../../src/format/formatter" -const node = NodeChildProcessSpawner.layer.pipe( - Layer.provideMerge(Layer.mergeAll(NodeFileSystem.layer, NodePath.layer)), -) - -const it = testEffect(Layer.mergeAll(Format.layer, node).pipe(Layer.provide(Config.defaultLayer))) +const it = testEffect(Format.defaultLayer) describe("Format", () => { it.effect("status() returns built-in formatters when no config overrides", () => From 09ce30c4335209f8d15f490128494d8d1bc3556f Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Fri, 27 Mar 2026 16:57:17 -0400 Subject: [PATCH 2/3] fix(format): restore format test dependencies --- packages/opencode/test/format/format.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/opencode/test/format/format.test.ts b/packages/opencode/test/format/format.test.ts index cc7f55aaf1..74336e02a3 100644 --- a/packages/opencode/test/format/format.test.ts +++ b/packages/opencode/test/format/format.test.ts @@ -1,11 +1,13 @@ +import { NodeFileSystem } from "@effect/platform-node" import { describe, expect } from "bun:test" -import { Effect } from "effect" +import { Effect, Layer } from "effect" import { provideTmpdirInstance } from "../fixture/fixture" import { testEffect } from "../lib/effect" +import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" import { Format } from "../../src/format" import * as Formatter from "../../src/format/formatter" -const it = testEffect(Format.defaultLayer) +const it = testEffect(Layer.mergeAll(Format.defaultLayer, CrossSpawnSpawner.defaultLayer, NodeFileSystem.layer)) describe("Format", () => { it.effect("status() returns built-in formatters when no config overrides", () => From 8a5a6852ca510ee7d283ea325f1ab2d581224ca2 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Fri, 27 Mar 2026 19:43:39 -0400 Subject: [PATCH 3/3] fix(format): set stdin/stdout/stderr to ignore for formatter subprocesses ChildProcess.make defaults to "pipe" for all stdio streams. Formatters that wait on stdin EOF or write enough output to fill pipe buffers would hang. Matches the original Process.spawn behavior. --- packages/opencode/src/format/index.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/opencode/src/format/index.ts b/packages/opencode/src/format/index.ts index 47b7d76b77..77577473f2 100644 --- a/packages/opencode/src/format/index.ts +++ b/packages/opencode/src/format/index.ts @@ -114,6 +114,9 @@ export namespace Format { cwd: Instance.directory, env: item.environment, extendEnv: true, + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", }), ) .pipe(