refactor: destroy Storage facades (#21956)

Author: kitlangton

packages/opencode/specs/effect-migration.md

@@ -308,3 +308,32 @@ Current raw fs users that will convert during tool migration:
 - [ ] `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
+
+## Destroying the facades
+
+Every service currently exports async facade functions at the bottom of its namespace — `export async function read(...) { return runPromise(...) }` — backed by a per-service `makeRuntime`. These exist because cyclic imports used to force each service to build its own independent runtime. Now that the layer DAG is acyclic and `AppRuntime` (`src/effect/app-runtime.ts`) composes everything into one `ManagedRuntime`, we're removing them.
+
+### Process
+
+For each service, the migration is roughly:
+
+1. **Find callers.** `grep -n "Namespace\.(methodA|methodB|...)"` across `src/` and `test/`. Skip the service file itself.
+2. **Migrate production callers.** For each effectful caller that does `Effect.tryPromise(() => Namespace.method(...))`:
+   - Add the service to the caller's layer R type (`Layer.Layer<Self, never, ... | Namespace.Service>`)
+   - Yield it at the top of the layer: `const ns = yield* Namespace.Service`
+   - Replace `Effect.tryPromise(() => Namespace.method(...))` with `yield* ns.method(...)` (or `ns.method(...).pipe(Effect.orElseSucceed(...))` for the common fallback case)
+   - Add `Layer.provide(Namespace.defaultLayer)` to the caller's own `defaultLayer` chain
+3. **Fix tests that used the caller's raw `.layer`.** Any test that composes `Caller.layer` (not `defaultLayer`) needs to also provide the newly-required service tag. The fastest fix is usually switching to `Caller.defaultLayer` since it now pulls in the new dependency.
+4. **Migrate test callers of the facade.** Tests calling `Namespace.method(...)` directly get converted to full effectful style using `testEffect(Namespace.defaultLayer)` + `it.live` / `it.effect` + `yield* svc.method(...)`. Don't wrap the test body in `Effect.promise(async () => {...})` — do the whole thing in `Effect.gen` and use `AppFileSystem.Service` / `tmpdirScoped` / `Effect.addFinalizer` for what used to be raw `fs` / `Bun.write` / `try/finally`.
+5. **Delete the facades.** Once `grep` shows zero callers, remove the `export async function` block AND the `makeRuntime(...)` line from the service namespace. Also remove the now-unused `import { makeRuntime }`.
+
+### Pitfalls
+
+- **Layer caching inside tests.** `testEffect(layer)` constructs the Storage (or whatever) service once and memoizes it. If a test then tries `inner.pipe(Effect.provide(customStorage))` to swap in a differently-configured Storage, the outer cached one wins and the inner provision is a no-op. Fix: wrap the overriding layer in `Layer.fresh(...)`, which forces a new instance to be built instead of hitting the memoMap cache. This lets a single `testEffect(...)` serve both simple and per-test-customized cases.
+- **`Effect.tryPromise` → `yield*` drops the Promise layer.** The old code was `Effect.tryPromise(() => Storage.read(...))` — a `tryPromise` wrapper because the facade returned a Promise. The new code is `yield* storage.read(...)` directly — the service method already returns an Effect, so no wrapper is needed. Don't reach for `Effect.promise` or `Effect.tryPromise` during migration; if you're using them on a service method call, you're doing it wrong.
+- **Raw `.layer` test callers break silently in the type checker.** When you add a new R requirement to a service's `.layer`, any test that composes it raw (not `defaultLayer`) becomes under-specified. `tsgo` will flag this — the error looks like `Type 'Storage.Service' is not assignable to type '... | Service | TestConsole'`. Usually the fix is to switch that composition to `defaultLayer`, or add `Layer.provide(NewDep.defaultLayer)` to the custom composition.
+- **Tests that do async setup with `fs`, `Bun.write`, `tmpdir`.** Convert these to `AppFileSystem.Service` calls inside `Effect.gen`, and use `tmpdirScoped()` instead of `tmpdir()` so cleanup happens via the scope finalizer. For file operations on the actual filesystem (not via a service), a small helper like `const writeJson = Effect.fnUntraced(function* (file, value) { const fs = yield* AppFileSystem.Service; yield* fs.makeDirectory(path.dirname(file), { recursive: true }); yield* fs.writeFileString(file, JSON.stringify(value, null, 2)) })` keeps the migration tests clean.
+
+### Migration log
+
+- `Storage` — migrated 2026-04-10. One production caller (`Session.diff`) and all storage.test.ts tests converted to effectful style. Facades and `makeRuntime` removed.

packages/opencode/src/session/index.ts

@@ -361,10 +361,11 @@ export namespace Session {
   const db = <T>(fn: (d: Parameters<typeof Database.use>[0] extends (trx: infer D) => any ? D : never) => T) =>
     Effect.sync(() => Database.use(fn))
 
-  export const layer: Layer.Layer<Service, never, Bus.Service> = Layer.effect(
+  export const layer: Layer.Layer<Service, never, Bus.Service | Storage.Service> = Layer.effect(
     Service,
     Effect.gen(function* () {
       const bus = yield* Bus.Service
+      const storage = yield* Storage.Service
 
       const createNext = Effect.fn("Session.createNext")(function* (input: {
         id?: SessionID
@@ -585,7 +586,7 @@ export namespace Session {
       })
 
       const diff = Effect.fn("Session.diff")(function* (sessionID: SessionID) {
-        return yield* Effect.tryPromise(() => Storage.read<Snapshot.FileDiff[]>(["session_diff", sessionID])).pipe(
+        return yield* storage.read<Snapshot.FileDiff[]>(["session_diff", sessionID]).pipe(
           Effect.orElseSucceed((): Snapshot.FileDiff[] => []),
         )
       })
@@ -660,7 +661,7 @@ export namespace Session {
     }),
   )
 
-  export const defaultLayer = layer.pipe(Layer.provide(Bus.layer))
+  export const defaultLayer = layer.pipe(Layer.provide(Bus.layer), Layer.provide(Storage.defaultLayer))
 
   const { runPromise } = makeRuntime(Service, defaultLayer)
 

packages/opencode/src/storage/storage.ts

@@ -4,7 +4,6 @@ import { Global } from "../global"
 import { NamedError } from "@opencode-ai/util/error"
 import z from "zod"
 import { AppFileSystem } from "@/filesystem"
-import { makeRuntime } from "@/effect/run-service"
 import { Effect, Exit, Layer, Option, RcMap, Schema, ServiceMap, TxReentrantLock } from "effect"
 import { Git } from "@/git"
 
@@ -331,26 +330,4 @@ export namespace Storage {
   )
 
   export const defaultLayer = layer.pipe(Layer.provide(AppFileSystem.defaultLayer), Layer.provide(Git.defaultLayer))
-
-  const { runPromise } = makeRuntime(Service, defaultLayer)
-
-  export async function remove(key: string[]) {
-    return runPromise((svc) => svc.remove(key))
-  }
-
-  export async function read<T>(key: string[]) {
-    return runPromise((svc) => svc.read<T>(key))
-  }
-
-  export async function update<T>(key: string[], fn: (draft: T) => void) {
-    return runPromise((svc) => svc.update<T>(key, fn))
-  }
-
-  export async function write<T>(key: string[], content: T) {
-    return runPromise((svc) => svc.write(key, content))
-  }
-
-  export async function list(prefix: string[]) {
-    return runPromise((svc) => svc.list(prefix))
-  }
 }

packages/opencode/test/share/share-next.test.ts

@@ -13,6 +13,7 @@ import { Provider } from "../../src/provider/provider"
 import { Session } from "../../src/session"
 import type { SessionID } from "../../src/session/schema"
 import { ShareNext } from "../../src/share/share-next"
+import { Storage } from "../../src/storage/storage"
 import { SessionShareTable } from "../../src/share/share.sql"
 import { Database, eq } from "../../src/storage/db"
 import { provideTmpdirInstance } from "../fixture/fixture"
@@ -55,7 +56,7 @@ function wired(client: HttpClient.HttpClient) {
   return Layer.mergeAll(
     Bus.layer,
     ShareNext.layer,
-    Session.layer,
+    Session.defaultLayer,
     AccountRepo.layer,
     NodeFileSystem.layer,
     CrossSpawnSpawner.defaultLayer,