refactor(tool): make Tool.Info init effectful (#21989)
This commit is contained in:
@@ -229,24 +229,24 @@ Still open:
|
||||
|
||||
## 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:
|
||||
`Tool.Def.execute` and `Tool.Info.init` already return `Effect` on this branch. Tool definitions should now stay Effect-native all the way through initialization instead of using Promise-returning init callbacks. Tools can still use lazy init callbacks when they need instance-bound state at init time, but those callbacks should return `Effect`, not `Promise`. Remaining work is:
|
||||
|
||||
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
|
||||
1. Migrate each tool body to return Effects
|
||||
2. Keep `Tool.define()` inputs Effect-native
|
||||
3. Update remaining callers to `yield*` tool initialization instead of `await`ing
|
||||
|
||||
### Tool migration details
|
||||
|
||||
Until the tool interface itself returns `Effect`, use this transitional pattern for migrated tools:
|
||||
With `Tool.Info.init()` now effectful, use this transitional pattern for migrated tools that still need Promise-based boundaries internally:
|
||||
|
||||
- `Tool.defineEffect(...)` should `yield*` the services the tool depends on and close over them in the returned tool definition.
|
||||
- Keep the bridge at the Promise boundary only. Prefer a single `Effect.runPromise(...)` in the temporary `async execute(...)` implementation, and move the inner logic into `Effect.fn(...)` helpers instead of scattering `runPromise` islands through the tool body.
|
||||
- Keep the bridge at the Promise boundary only inside the tool body when required by external APIs. Do not return Promise-based init callbacks from `Tool.define()`.
|
||||
- If a tool starts requiring new services, wire them into `ToolRegistry.defaultLayer` so production callers resolve the same dependencies as tests.
|
||||
|
||||
Tool tests should use the existing Effect helpers in `packages/opencode/test/lib/effect.ts`:
|
||||
|
||||
- Use `testEffect(...)` / `it.live(...)` instead of creating fake local wrappers around effectful tools.
|
||||
- Yield the real tool export, then initialize it: `const info = yield* ReadTool`, `const tool = yield* Effect.promise(() => info.init())`.
|
||||
- Yield the real tool export, then initialize it: `const info = yield* ReadTool`, `const tool = yield* info.init()`.
|
||||
- Run tests inside a real instance with `provideTmpdirInstance(...)` or `provideInstance(tmpdirScoped(...))` so instance-scoped services resolve exactly as they do in production.
|
||||
|
||||
This keeps migrated tool tests aligned with the production service graph today, and makes the eventual `Tool.Info` → `Effect` cleanup mostly mechanical later.
|
||||
|
||||
@@ -454,7 +454,8 @@ export const BashTool = Tool.define(
|
||||
}
|
||||
})
|
||||
|
||||
return async () => {
|
||||
return () =>
|
||||
Effect.sync(() => {
|
||||
const shell = Shell.acceptable()
|
||||
const name = Shell.name(shell)
|
||||
const chain =
|
||||
@@ -500,6 +501,6 @@ export const BashTool = Tool.define(
|
||||
)
|
||||
}),
|
||||
}
|
||||
}
|
||||
})
|
||||
}),
|
||||
)
|
||||
|
||||
@@ -10,7 +10,7 @@ export const MultiEditTool = Tool.define(
|
||||
"multiedit",
|
||||
Effect.gen(function* () {
|
||||
const editInfo = yield* EditTool
|
||||
const edit = yield* Effect.promise(() => editInfo.init())
|
||||
const edit = yield* editInfo.init()
|
||||
|
||||
return {
|
||||
description: DESCRIPTION,
|
||||
|
||||
@@ -17,9 +17,9 @@ export const SkillTool = Tool.define(
|
||||
Effect.gen(function* () {
|
||||
const skill = yield* Skill.Service
|
||||
const rg = yield* Ripgrep.Service
|
||||
|
||||
return async () => {
|
||||
const list = await Effect.runPromise(skill.available().pipe(Effect.provide(EffectLogger.layer)))
|
||||
return () =>
|
||||
Effect.gen(function* () {
|
||||
const list = yield* skill.available().pipe(Effect.provide(EffectLogger.layer))
|
||||
|
||||
const description =
|
||||
list.length === 0
|
||||
@@ -95,6 +95,6 @@ export const SkillTool = Tool.define(
|
||||
}
|
||||
}).pipe(Effect.orDie),
|
||||
}
|
||||
}
|
||||
})
|
||||
}),
|
||||
)
|
||||
|
||||
@@ -47,9 +47,13 @@ export namespace Tool {
|
||||
|
||||
export interface Info<Parameters extends z.ZodType = z.ZodType, M extends Metadata = Metadata> {
|
||||
id: string
|
||||
init: () => Promise<DefWithoutID<Parameters, M>>
|
||||
init: () => Effect.Effect<DefWithoutID<Parameters, M>>
|
||||
}
|
||||
|
||||
type Init<Parameters extends z.ZodType, M extends Metadata> =
|
||||
| DefWithoutID<Parameters, M>
|
||||
| (() => Effect.Effect<DefWithoutID<Parameters, M>>)
|
||||
|
||||
export type InferParameters<T> =
|
||||
T extends Info<infer P, any>
|
||||
? z.infer<P>
|
||||
@@ -66,12 +70,10 @@ export namespace Tool {
|
||||
? Def<P, M>
|
||||
: never
|
||||
|
||||
function wrap<Parameters extends z.ZodType, Result extends Metadata>(
|
||||
id: string,
|
||||
init: (() => Promise<DefWithoutID<Parameters, Result>>) | DefWithoutID<Parameters, Result>,
|
||||
) {
|
||||
return async () => {
|
||||
const toolInfo = init instanceof Function ? await init() : { ...init }
|
||||
function wrap<Parameters extends z.ZodType, Result extends Metadata>(id: string, init: Init<Parameters, Result>) {
|
||||
return () =>
|
||||
Effect.gen(function* () {
|
||||
const toolInfo = init instanceof Function ? { ...(yield* init()) } : { ...init }
|
||||
const execute = toolInfo.execute
|
||||
toolInfo.execute = (args, ctx) =>
|
||||
Effect.gen(function* () {
|
||||
@@ -104,22 +106,22 @@ export namespace Tool {
|
||||
}
|
||||
}).pipe(Effect.orDie)
|
||||
return toolInfo
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
export function define<Parameters extends z.ZodType, Result extends Metadata, R, ID extends string = string>(
|
||||
id: ID,
|
||||
init: Effect.Effect<(() => Promise<DefWithoutID<Parameters, Result>>) | DefWithoutID<Parameters, Result>, never, R>,
|
||||
init: Effect.Effect<Init<Parameters, Result>, never, R>,
|
||||
): Effect.Effect<Info<Parameters, Result>, never, R> & { id: ID } {
|
||||
return Object.assign(
|
||||
Effect.map(init, (next) => ({ id, init: wrap(id, next) })),
|
||||
Effect.map(init, (init) => ({ id, init: wrap(id, init) })),
|
||||
{ id },
|
||||
)
|
||||
}
|
||||
|
||||
export function init<P extends z.ZodType, M extends Metadata>(info: Info<P, M>): Effect.Effect<Def<P, M>> {
|
||||
return Effect.gen(function* () {
|
||||
const init = yield* Effect.promise(() => info.init())
|
||||
const init = yield* info.init()
|
||||
return {
|
||||
...init,
|
||||
id: info.id,
|
||||
|
||||
@@ -50,7 +50,7 @@ type ToolCtx = typeof baseCtx & {
|
||||
|
||||
const execute = async (params: { patchText: string }, ctx: ToolCtx) => {
|
||||
const info = await runtime.runPromise(ApplyPatchTool)
|
||||
const tool = await info.init()
|
||||
const tool = await runtime.runPromise(info.init())
|
||||
return Effect.runPromise(tool.execute(params, ctx))
|
||||
}
|
||||
|
||||
|
||||
@@ -19,7 +19,7 @@ const runtime = ManagedRuntime.make(
|
||||
)
|
||||
|
||||
function initBash() {
|
||||
return runtime.runPromise(BashTool.pipe(Effect.flatMap((info) => Effect.promise(() => info.init()))))
|
||||
return runtime.runPromise(BashTool.pipe(Effect.flatMap((info) => info.init())))
|
||||
}
|
||||
|
||||
const ctx = {
|
||||
|
||||
@@ -45,7 +45,7 @@ const resolve = () =>
|
||||
runtime.runPromise(
|
||||
Effect.gen(function* () {
|
||||
const info = yield* EditTool
|
||||
return yield* Effect.promise(() => info.init())
|
||||
return yield* info.init()
|
||||
}),
|
||||
)
|
||||
|
||||
|
||||
@@ -10,7 +10,7 @@ import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner"
|
||||
const runtime = ManagedRuntime.make(Layer.mergeAll(CrossSpawnSpawner.defaultLayer))
|
||||
|
||||
function initGrep() {
|
||||
return runtime.runPromise(GrepTool.pipe(Effect.flatMap((info) => Effect.promise(() => info.init()))))
|
||||
return runtime.runPromise(GrepTool.pipe(Effect.flatMap((info) => info.init())))
|
||||
}
|
||||
|
||||
const ctx = {
|
||||
|
||||
@@ -36,7 +36,7 @@ describe("tool.question", () => {
|
||||
Effect.gen(function* () {
|
||||
const question = yield* Question.Service
|
||||
const toolInfo = yield* QuestionTool
|
||||
const tool = yield* Effect.promise(() => toolInfo.init())
|
||||
const tool = yield* toolInfo.init()
|
||||
const questions = [
|
||||
{
|
||||
question: "What is your favorite color?",
|
||||
@@ -64,7 +64,7 @@ describe("tool.question", () => {
|
||||
Effect.gen(function* () {
|
||||
const question = yield* Question.Service
|
||||
const toolInfo = yield* QuestionTool
|
||||
const tool = yield* Effect.promise(() => toolInfo.init())
|
||||
const tool = yield* toolInfo.init()
|
||||
const questions = [
|
||||
{
|
||||
question: "What is your favorite animal?",
|
||||
|
||||
@@ -46,7 +46,7 @@ const it = testEffect(
|
||||
|
||||
const init = Effect.fn("ReadToolTest.init")(function* () {
|
||||
const info = yield* ReadTool
|
||||
return yield* Effect.promise(() => info.init())
|
||||
return yield* info.init()
|
||||
})
|
||||
|
||||
const run = Effect.fn("ReadToolTest.run")(function* (
|
||||
|
||||
@@ -152,7 +152,7 @@ Use this skill.
|
||||
fn: async () => {
|
||||
const runtime = ManagedRuntime.make(Layer.mergeAll(Skill.defaultLayer, Ripgrep.defaultLayer))
|
||||
const info = await runtime.runPromise(SkillTool)
|
||||
const tool = await info.init()
|
||||
const tool = await runtime.runPromise(info.init())
|
||||
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
|
||||
const ctx: Tool.Context = {
|
||||
...baseCtx,
|
||||
|
||||
@@ -191,7 +191,7 @@ describe("tool.task", () => {
|
||||
const { chat, assistant } = yield* seed()
|
||||
const child = yield* sessions.create({ parentID: chat.id, title: "Existing child" })
|
||||
const tool = yield* TaskTool
|
||||
const def = yield* Effect.promise(() => tool.init())
|
||||
const def = yield* tool.init()
|
||||
let seen: SessionPrompt.PromptInput | undefined
|
||||
const promptOps = stubOps({ text: "resumed", onPrompt: (input) => (seen = input) })
|
||||
|
||||
@@ -229,7 +229,7 @@ describe("tool.task", () => {
|
||||
Effect.gen(function* () {
|
||||
const { chat, assistant } = yield* seed()
|
||||
const tool = yield* TaskTool
|
||||
const def = yield* Effect.promise(() => tool.init())
|
||||
const def = yield* tool.init()
|
||||
const calls: unknown[] = []
|
||||
const promptOps = stubOps()
|
||||
|
||||
@@ -278,7 +278,7 @@ describe("tool.task", () => {
|
||||
const sessions = yield* Session.Service
|
||||
const { chat, assistant } = yield* seed()
|
||||
const tool = yield* TaskTool
|
||||
const def = yield* Effect.promise(() => tool.init())
|
||||
const def = yield* tool.init()
|
||||
let seen: SessionPrompt.PromptInput | undefined
|
||||
const promptOps = stubOps({ text: "created", onPrompt: (input) => (seen = input) })
|
||||
|
||||
@@ -318,7 +318,7 @@ describe("tool.task", () => {
|
||||
const sessions = yield* Session.Service
|
||||
const { chat, assistant } = yield* seed()
|
||||
const tool = yield* TaskTool
|
||||
const def = yield* Effect.promise(() => tool.init())
|
||||
const def = yield* tool.init()
|
||||
let seen: SessionPrompt.PromptInput | undefined
|
||||
const promptOps = stubOps({ onPrompt: (input) => (seen = input) })
|
||||
|
||||
|
||||
@@ -23,23 +23,23 @@ describe("Tool.define", () => {
|
||||
|
||||
const info = await Effect.runPromise(Tool.define("test-tool", Effect.succeed(original)))
|
||||
|
||||
await info.init()
|
||||
await info.init()
|
||||
await info.init()
|
||||
await Effect.runPromise(info.init())
|
||||
await Effect.runPromise(info.init())
|
||||
await Effect.runPromise(info.init())
|
||||
|
||||
expect(original.execute).toBe(originalExecute)
|
||||
})
|
||||
|
||||
test("function-defined tool returns fresh objects and is unaffected", async () => {
|
||||
test("effect-defined tool returns fresh objects and is unaffected", async () => {
|
||||
const info = await Effect.runPromise(
|
||||
Tool.define(
|
||||
"test-fn-tool",
|
||||
Effect.succeed(() => Promise.resolve(makeTool("test"))),
|
||||
Effect.succeed(() => Effect.succeed(makeTool("test"))),
|
||||
),
|
||||
)
|
||||
|
||||
const first = await info.init()
|
||||
const second = await info.init()
|
||||
const first = await Effect.runPromise(info.init())
|
||||
const second = await Effect.runPromise(info.init())
|
||||
|
||||
expect(first).not.toBe(second)
|
||||
})
|
||||
@@ -47,8 +47,8 @@ describe("Tool.define", () => {
|
||||
test("object-defined tool returns distinct objects per init() call", async () => {
|
||||
const info = await Effect.runPromise(Tool.define("test-copy", Effect.succeed(makeTool("test"))))
|
||||
|
||||
const first = await info.init()
|
||||
const second = await info.init()
|
||||
const first = await Effect.runPromise(info.init())
|
||||
const second = await Effect.runPromise(info.init())
|
||||
|
||||
expect(first).not.toBe(second)
|
||||
})
|
||||
|
||||
@@ -26,7 +26,7 @@ async function withFetch(fetch: (req: Request) => Response | Promise<Response>,
|
||||
|
||||
function initTool() {
|
||||
return WebFetchTool.pipe(
|
||||
Effect.flatMap((info) => Effect.promise(() => info.init())),
|
||||
Effect.flatMap((info) => info.init()),
|
||||
Effect.provide(FetchHttpClient.layer),
|
||||
Effect.runPromise,
|
||||
)
|
||||
|
||||
@@ -43,7 +43,7 @@ const it = testEffect(
|
||||
|
||||
const init = Effect.fn("WriteToolTest.init")(function* () {
|
||||
const info = yield* WriteTool
|
||||
return yield* Effect.promise(() => info.init())
|
||||
return yield* info.init()
|
||||
})
|
||||
|
||||
const run = Effect.fn("WriteToolTest.run")(function* (
|
||||
|
||||
Reference in New Issue
Block a user