refactor(effect): build todowrite tool from Todo service (#20789)

Co-authored-by: Juan Pablo Carranza Hurtado <52012198+jpcarranza94@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
pull/20918/head
Kit Langton 2026-04-03 12:05:40 -04:00 committed by GitHub
parent ae7e2eb3fb
commit f2d4ced8ea
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 217 additions and 194 deletions

View File

@ -82,7 +82,7 @@ export namespace Todo {
}), }),
) )
const defaultLayer = layer.pipe(Layer.provide(Bus.layer)) export const defaultLayer = layer.pipe(Layer.provide(Bus.layer))
const { runPromise } = makeRuntime(Service, defaultLayer) const { runPromise } = makeRuntime(Service, defaultLayer)
export async function update(input: { sessionID: SessionID; todos: Info[] }) { export async function update(input: { sessionID: SessionID; todos: Info[] }) {

View File

@ -34,6 +34,7 @@ import { InstanceState } from "@/effect/instance-state"
import { makeRuntime } from "@/effect/run-service" import { makeRuntime } from "@/effect/run-service"
import { Env } from "../env" import { Env } from "../env"
import { Question } from "../question" import { Question } from "../question"
import { Todo } from "../session/todo"
export namespace ToolRegistry { export namespace ToolRegistry {
const log = Log.create({ service: "tool.registry" }) const log = Log.create({ service: "tool.registry" })
@ -56,7 +57,8 @@ export namespace ToolRegistry {
export class Service extends ServiceMap.Service<Service, Interface>()("@opencode/ToolRegistry") {} export class Service extends ServiceMap.Service<Service, Interface>()("@opencode/ToolRegistry") {}
export const layer: Layer.Layer<Service, never, Config.Service | Plugin.Service | Question.Service> = Layer.effect( export const layer: Layer.Layer<Service, never, Config.Service | Plugin.Service | Question.Service | Todo.Service> =
Layer.effect(
Service, Service,
Effect.gen(function* () { Effect.gen(function* () {
const config = yield* Config.Service const config = yield* Config.Service
@ -140,7 +142,8 @@ export namespace ToolRegistry {
const all = Effect.fn("ToolRegistry.all")(function* (custom: Tool.Info[]) { const all = Effect.fn("ToolRegistry.all")(function* (custom: Tool.Info[]) {
const cfg = yield* config.get() const cfg = yield* config.get()
const question = ["app", "cli", "desktop"].includes(Flag.OPENCODE_CLIENT) || Flag.OPENCODE_ENABLE_QUESTION_TOOL const question =
["app", "cli", "desktop"].includes(Flag.OPENCODE_CLIENT) || Flag.OPENCODE_ENABLE_QUESTION_TOOL
return [ return [
invalid, invalid,
@ -222,6 +225,7 @@ export namespace ToolRegistry {
Layer.provide(Config.defaultLayer), Layer.provide(Config.defaultLayer),
Layer.provide(Plugin.defaultLayer), Layer.provide(Plugin.defaultLayer),
Layer.provide(Question.defaultLayer), Layer.provide(Question.defaultLayer),
Layer.provide(Todo.defaultLayer),
), ),
), ),
) )

View File

@ -1,14 +1,26 @@
import z from "zod" import z from "zod"
import { Effect } from "effect"
import { Tool } from "./tool" import { Tool } from "./tool"
import DESCRIPTION_WRITE from "./todowrite.txt" import DESCRIPTION_WRITE from "./todowrite.txt"
import { Todo } from "../session/todo" import { Todo } from "../session/todo"
export const TodoWriteTool = Tool.define("todowrite", { const parameters = z.object({
description: DESCRIPTION_WRITE,
parameters: z.object({
todos: z.array(z.object(Todo.Info.shape)).describe("The updated todo list"), todos: z.array(z.object(Todo.Info.shape)).describe("The updated todo list"),
}), })
async execute(params, ctx) {
type Metadata = {
todos: Todo.Info[]
}
export const TodoWriteTool = Tool.defineEffect<typeof parameters, Metadata, Todo.Service>(
"todowrite",
Effect.gen(function* () {
const todo = yield* Todo.Service
return {
description: DESCRIPTION_WRITE,
parameters,
async execute(params: z.infer<typeof parameters>, ctx: Tool.Context<Metadata>) {
await ctx.ask({ await ctx.ask({
permission: "todowrite", permission: "todowrite",
patterns: ["*"], patterns: ["*"],
@ -16,10 +28,13 @@ export const TodoWriteTool = Tool.define("todowrite", {
metadata: {}, metadata: {},
}) })
await Todo.update({ await todo
.update({
sessionID: ctx.sessionID, sessionID: ctx.sessionID,
todos: params.todos, todos: params.todos,
}) })
.pipe(Effect.runPromise)
return { return {
title: `${params.todos.filter((x) => x.status !== "completed").length} todos`, title: `${params.todos.filter((x) => x.status !== "completed").length} todos`,
output: JSON.stringify(params.todos, null, 2), output: JSON.stringify(params.todos, null, 2),
@ -28,4 +43,6 @@ export const TodoWriteTool = Tool.define("todowrite", {
}, },
} }
}, },
}) } satisfies Tool.Def<typeof parameters, Metadata>
}),
)

View File

@ -16,6 +16,7 @@ import { Provider as ProviderSvc } from "../../src/provider/provider"
import type { Provider } from "../../src/provider/provider" import type { Provider } from "../../src/provider/provider"
import { ModelID, ProviderID } from "../../src/provider/schema" import { ModelID, ProviderID } from "../../src/provider/schema"
import { Question } from "../../src/question" import { Question } from "../../src/question"
import { Todo } from "../../src/session/todo"
import { Session } from "../../src/session" import { Session } from "../../src/session"
import { LLM } from "../../src/session/llm" import { LLM } from "../../src/session/llm"
import { MessageV2 } from "../../src/session/message-v2" import { MessageV2 } from "../../src/session/message-v2"
@ -162,7 +163,12 @@ function makeHttp() {
status, status,
).pipe(Layer.provideMerge(infra)) ).pipe(Layer.provideMerge(infra))
const question = Question.layer.pipe(Layer.provideMerge(deps)) const question = Question.layer.pipe(Layer.provideMerge(deps))
const registry = ToolRegistry.layer.pipe(Layer.provideMerge(question), Layer.provideMerge(deps)) const todo = Todo.layer.pipe(Layer.provideMerge(deps))
const registry = ToolRegistry.layer.pipe(
Layer.provideMerge(todo),
Layer.provideMerge(question),
Layer.provideMerge(deps),
)
const trunc = Truncate.layer.pipe(Layer.provideMerge(deps)) const trunc = Truncate.layer.pipe(Layer.provideMerge(deps))
const proc = SessionProcessor.layer.pipe(Layer.provideMerge(deps)) const proc = SessionProcessor.layer.pipe(Layer.provideMerge(deps))
const compact = SessionCompaction.layer.pipe(Layer.provideMerge(proc), Layer.provideMerge(deps)) const compact = SessionCompaction.layer.pipe(Layer.provideMerge(proc), Layer.provideMerge(deps))

View File

@ -39,6 +39,7 @@ import { Permission } from "../../src/permission"
import { Plugin } from "../../src/plugin" import { Plugin } from "../../src/plugin"
import { Provider as ProviderSvc } from "../../src/provider/provider" import { Provider as ProviderSvc } from "../../src/provider/provider"
import { Question } from "../../src/question" import { Question } from "../../src/question"
import { Todo } from "../../src/session/todo"
import { SessionCompaction } from "../../src/session/compaction" import { SessionCompaction } from "../../src/session/compaction"
import { Instruction } from "../../src/session/instruction" import { Instruction } from "../../src/session/instruction"
import { SessionProcessor } from "../../src/session/processor" import { SessionProcessor } from "../../src/session/processor"
@ -126,7 +127,12 @@ function makeHttp() {
status, status,
).pipe(Layer.provideMerge(infra)) ).pipe(Layer.provideMerge(infra))
const question = Question.layer.pipe(Layer.provideMerge(deps)) const question = Question.layer.pipe(Layer.provideMerge(deps))
const registry = ToolRegistry.layer.pipe(Layer.provideMerge(question), Layer.provideMerge(deps)) const todo = Todo.layer.pipe(Layer.provideMerge(deps))
const registry = ToolRegistry.layer.pipe(
Layer.provideMerge(todo),
Layer.provideMerge(question),
Layer.provideMerge(deps),
)
const trunc = Truncate.layer.pipe(Layer.provideMerge(deps)) const trunc = Truncate.layer.pipe(Layer.provideMerge(deps))
const proc = SessionProcessor.layer.pipe(Layer.provideMerge(deps)) const proc = SessionProcessor.layer.pipe(Layer.provideMerge(deps))
const compact = SessionCompaction.layer.pipe(Layer.provideMerge(proc), Layer.provideMerge(deps)) const compact = SessionCompaction.layer.pipe(Layer.provideMerge(proc), Layer.provideMerge(deps))

View File

@ -27,45 +27,37 @@ describe("Tool.define", () => {
await tool.init() await tool.init()
await tool.init() await tool.init()
// The original object's execute should never be overwritten
expect(original.execute).toBe(originalExecute) expect(original.execute).toBe(originalExecute)
}) })
test("object-defined tool does not accumulate wrapper layers across init() calls", async () => { test("object-defined tool does not accumulate wrapper layers across init() calls", async () => {
let executeCalls = 0 let calls = 0
const tool = Tool.define( const tool = Tool.define(
"test-tool", "test-tool",
makeTool("test", () => executeCalls++), makeTool("test", () => calls++),
) )
// Call init() many times to simulate many agentic steps
for (let i = 0; i < 100; i++) { for (let i = 0; i < 100; i++) {
await tool.init() await tool.init()
} }
// Resolve the tool and call execute
const resolved = await tool.init() const resolved = await tool.init()
executeCalls = 0 calls = 0
// Capture the stack trace inside execute to measure wrapper depth let stack = ""
let stackInsideExecute = "" const exec = resolved.execute
const origExec = resolved.execute
resolved.execute = async (args: any, ctx: any) => { resolved.execute = async (args: any, ctx: any) => {
const result = await origExec.call(resolved, args, ctx) const result = await exec.call(resolved, args, ctx)
const err = new Error() stack = new Error().stack || ""
stackInsideExecute = err.stack || ""
return result return result
} }
await resolved.execute(defaultArgs, {} as any) await resolved.execute(defaultArgs, {} as any)
expect(executeCalls).toBe(1) expect(calls).toBe(1)
// Count how many times tool.ts appears in the stack. const frames = stack.split("\n").filter((l) => l.includes("tool.ts")).length
// With the fix: 1 wrapper layer (from the most recent init()). expect(frames).toBeLessThan(5)
// Without the fix: 101 wrapper layers from accumulated closures.
const toolTsFrames = stackInsideExecute.split("\n").filter((l) => l.includes("tool.ts")).length
expect(toolTsFrames).toBeLessThan(5)
}) })
test("function-defined tool returns fresh objects and is unaffected", async () => { test("function-defined tool returns fresh objects and is unaffected", async () => {
@ -74,7 +66,6 @@ describe("Tool.define", () => {
const first = await tool.init() const first = await tool.init()
const second = await tool.init() const second = await tool.init()
// Function-defined tools return distinct objects each time
expect(first).not.toBe(second) expect(first).not.toBe(second)
}) })
@ -84,7 +75,6 @@ describe("Tool.define", () => {
const first = await tool.init() const first = await tool.init()
const second = await tool.init() const second = await tool.init()
// Each init() should return a separate object so wrappers don't accumulate
expect(first).not.toBe(second) expect(first).not.toBe(second)
}) })