Explorar o código

fix(cli): bridge Instance.current ALS in effectCmd handlers (regression from #25522) (#25546)

Kit Langton hai 4 meses
pai
achega
2df8eda8a3

+ 16 - 11
packages/opencode/src/cli/effect-cmd.ts

@@ -3,6 +3,7 @@ import { Effect, Schema } from "effect"
 import { AppRuntime, type AppServices } from "@/effect/app-runtime"
 import { InstanceStore } from "@/project/instance-store"
 import { InstanceRef } from "@/effect/instance-ref"
+import { Instance } from "@/project/instance"
 import { cmd, type WithDoubleDash } from "./cmd/cmd"
 
 /**
@@ -82,17 +83,21 @@ export const effectCmd = <Args, A>(opts: EffectCmdOpts<Args, A>) =>
         return
       }
       const directory = opts.directory?.(args) ?? process.cwd()
-      await AppRuntime.runPromise(
-        InstanceStore.Service.use((store) =>
-          store.provide(
-            { directory },
-            Effect.gen(function* () {
-              const ctx = yield* InstanceRef
-              const body = opts.handler(args)
-              return ctx ? yield* body.pipe(Effect.ensuring(store.dispose(ctx))) : yield* body
-            }),
-          ),
-        ),
+      // Two-phase: load ctx, then run body inside Instance.current ALS.
+      // Effect's InstanceRef is provided via fiber context, but that context is
+      // lost across `await` inside `Effect.promise(async () => ...)` callbacks
+      // — when handlers re-enter Effect via `AppRuntime.runPromise(svc.method())`
+      // there, attach() falls back to Instance.current ALS, which Node preserves
+      // across awaits. Matches the pre-effectCmd `bootstrap()` behavior.
+      const { store, ctx } = await AppRuntime.runPromise(
+        InstanceStore.Service.use((store) => store.load({ directory }).pipe(Effect.map((ctx) => ({ store, ctx })))),
       )
+      try {
+        await Instance.restore(ctx, () =>
+          AppRuntime.runPromise(opts.handler(args).pipe(Effect.provideService(InstanceRef, ctx))),
+        )
+      } finally {
+        await AppRuntime.runPromise(store.dispose(ctx))
+      }
     },
   })

+ 48 - 0
packages/opencode/test/cli/effect-cmd-instance-als.test.ts

@@ -0,0 +1,48 @@
+import { afterEach, expect, test } from "bun:test"
+import { Effect } from "effect"
+import fs from "fs/promises"
+import { Instance } from "../../src/project/instance"
+import { disposeAllInstances, provideTestInstance, tmpdir } from "../fixture/fixture"
+
+afterEach(async () => {
+  await disposeAllInstances()
+})
+
+// Regression for PR #25522: when an effectCmd handler does
+// `yield* Effect.promise(async () => { ... await runPromise(svcMethod) ... })`,
+// the inner runPromise creates a fresh fiber after `await` whose Effect context
+// has lost the outer InstanceRef. Services that read `InstanceState.context`
+// then fall back to `Instance.current` ALS, which must be installed at the JS
+// callback boundary (Node ALS persists across awaits, Effect's fiber context
+// does not). `provideTestInstance` mirrors effectCmd's load + ALS-restore wrap.
+// Pins effect-cmd.ts directly: the pattern test below exercises the load +
+// Instance.restore + dispose triple via the shared `provideTestInstance` fixture,
+// so a regression that removed `Instance.restore` from effect-cmd.ts wouldn't
+// fail it. This grep guards the actual production callsite.
+test("effect-cmd.ts wraps the handler body in Instance.restore", async () => {
+  const source = await fs.readFile(new URL("../../src/cli/effect-cmd.ts", import.meta.url), "utf8")
+  expect(source).toContain("Instance.restore(ctx")
+})
+
+test("Instance.current reachable from inner runPromise inside Effect.promise(async)", async () => {
+  await using dir = await tmpdir({ git: true })
+  await provideTestInstance({
+    directory: dir.path,
+    fn: () =>
+      Effect.runPromise(
+        Effect.promise(async () => {
+          await new Promise((r) => setTimeout(r, 5))
+          const current = await Effect.runPromise(
+            Effect.sync(() => {
+              try {
+                return Instance.current
+              } catch {
+                return undefined
+              }
+            }),
+          )
+          expect(current?.directory).toBe(dir.path)
+        }),
+      ),
+  })
+})