refactor(tool): migrate tool framework + all 18 built-in tools to Effect Schema (#23293)

This commit is contained in:
Kit Langton
2026-04-18 11:58:38 -04:00
committed by GitHub
parent d6abb56f58
commit a49b5adfbd
25 changed files with 319 additions and 240 deletions
+54 -51
View File
@@ -1,11 +1,13 @@
import { describe, expect, test } from "bun:test"
import z from "zod"
import { Result, Schema } from "effect"
import { toJsonSchema } from "../../src/util/effect-zod"
// Each tool exports its parameters schema at module scope so this test can
// import them without running the tool's Effect-based init. The JSON Schema
// snapshot captures what the LLM sees; the parse assertions pin down the
// accepts/rejects contract. Both must survive any future migration (e.g. from
// zod to Effect Schema via the effect-zod walker) byte-for-byte.
// accepts/rejects contract. `toJsonSchema` is the same helper `session/
// prompt.ts` uses to emit tool schemas to the LLM, so the snapshots stay
// byte-identical regardless of whether a tool has migrated from zod to Schema.
import { Parameters as ApplyPatch } from "../../src/tool/apply_patch"
import { Parameters as Bash } from "../../src/tool/bash"
@@ -26,10 +28,11 @@ import { Parameters as WebFetch } from "../../src/tool/webfetch"
import { Parameters as WebSearch } from "../../src/tool/websearch"
import { Parameters as Write } from "../../src/tool/write"
// Helper: the JSON Schema the LLM sees at tool registration time
// (session/prompt.ts runs `z.toJSONSchema(tool.parameters)` with the AI SDK's
// default `io` mode). Snapshots pin the exact wire shape.
const toJsonSchema = (schema: z.ZodType) => z.toJSONSchema(schema, { io: "input" })
const parse = <S extends Schema.Decoder<unknown>>(schema: S, input: unknown): S["Type"] =>
Schema.decodeUnknownSync(schema)(input)
const accepts = (schema: Schema.Decoder<unknown>, input: unknown): boolean =>
Result.isSuccess(Schema.decodeUnknownResult(schema)(input))
describe("tool parameters", () => {
describe("JSON Schema (wire shape)", () => {
@@ -55,53 +58,53 @@ describe("tool parameters", () => {
describe("apply_patch", () => {
test("accepts patchText", () => {
expect(ApplyPatch.parse({ patchText: "*** Begin Patch\n*** End Patch" })).toEqual({
expect(parse(ApplyPatch, { patchText: "*** Begin Patch\n*** End Patch" })).toEqual({
patchText: "*** Begin Patch\n*** End Patch",
})
})
test("rejects missing patchText", () => {
expect(ApplyPatch.safeParse({}).success).toBe(false)
expect(accepts(ApplyPatch, {})).toBe(false)
})
test("rejects non-string patchText", () => {
expect(ApplyPatch.safeParse({ patchText: 123 }).success).toBe(false)
expect(accepts(ApplyPatch, { patchText: 123 })).toBe(false)
})
})
describe("bash", () => {
test("accepts minimum: command + description", () => {
expect(Bash.parse({ command: "ls", description: "list" })).toEqual({ command: "ls", description: "list" })
expect(parse(Bash, { command: "ls", description: "list" })).toEqual({ command: "ls", description: "list" })
})
test("accepts optional timeout + workdir", () => {
const parsed = Bash.parse({ command: "ls", description: "list", timeout: 5000, workdir: "/tmp" })
const parsed = parse(Bash, { command: "ls", description: "list", timeout: 5000, workdir: "/tmp" })
expect(parsed.timeout).toBe(5000)
expect(parsed.workdir).toBe("/tmp")
})
test("rejects missing description (required by zod)", () => {
expect(Bash.safeParse({ command: "ls" }).success).toBe(false)
expect(accepts(Bash, { command: "ls" })).toBe(false)
})
test("rejects missing command", () => {
expect(Bash.safeParse({ description: "list" }).success).toBe(false)
expect(accepts(Bash, { description: "list" })).toBe(false)
})
})
describe("codesearch", () => {
test("accepts query; tokensNum defaults to 5000", () => {
expect(CodeSearch.parse({ query: "hooks" })).toEqual({ query: "hooks", tokensNum: 5000 })
expect(parse(CodeSearch, { query: "hooks" })).toEqual({ query: "hooks", tokensNum: 5000 })
})
test("accepts override tokensNum", () => {
expect(CodeSearch.parse({ query: "hooks", tokensNum: 10000 }).tokensNum).toBe(10000)
expect(parse(CodeSearch, { query: "hooks", tokensNum: 10000 }).tokensNum).toBe(10000)
})
test("rejects tokensNum under 1000", () => {
expect(CodeSearch.safeParse({ query: "x", tokensNum: 500 }).success).toBe(false)
expect(accepts(CodeSearch, { query: "x", tokensNum: 500 })).toBe(false)
})
test("rejects tokensNum over 50000", () => {
expect(CodeSearch.safeParse({ query: "x", tokensNum: 60000 }).success).toBe(false)
expect(accepts(CodeSearch, { query: "x", tokensNum: 60000 })).toBe(false)
})
})
describe("edit", () => {
test("accepts all four fields", () => {
expect(Edit.parse({ filePath: "/a", oldString: "x", newString: "y", replaceAll: true })).toEqual({
expect(parse(Edit, { filePath: "/a", oldString: "x", newString: "y", replaceAll: true })).toEqual({
filePath: "/a",
oldString: "x",
newString: "y",
@@ -109,72 +112,72 @@ describe("tool parameters", () => {
})
})
test("replaceAll is optional", () => {
const parsed = Edit.parse({ filePath: "/a", oldString: "x", newString: "y" })
const parsed = parse(Edit, { filePath: "/a", oldString: "x", newString: "y" })
expect(parsed.replaceAll).toBeUndefined()
})
test("rejects missing filePath", () => {
expect(Edit.safeParse({ oldString: "x", newString: "y" }).success).toBe(false)
expect(accepts(Edit, { oldString: "x", newString: "y" })).toBe(false)
})
})
describe("glob", () => {
test("accepts pattern-only", () => {
expect(Glob.parse({ pattern: "**/*.ts" })).toEqual({ pattern: "**/*.ts" })
expect(parse(Glob, { pattern: "**/*.ts" })).toEqual({ pattern: "**/*.ts" })
})
test("accepts optional path", () => {
expect(Glob.parse({ pattern: "**/*.ts", path: "/tmp" }).path).toBe("/tmp")
expect(parse(Glob, { pattern: "**/*.ts", path: "/tmp" }).path).toBe("/tmp")
})
test("rejects missing pattern", () => {
expect(Glob.safeParse({}).success).toBe(false)
expect(accepts(Glob, {})).toBe(false)
})
})
describe("grep", () => {
test("accepts pattern-only", () => {
expect(Grep.parse({ pattern: "TODO" })).toEqual({ pattern: "TODO" })
expect(parse(Grep, { pattern: "TODO" })).toEqual({ pattern: "TODO" })
})
test("accepts optional path + include", () => {
const parsed = Grep.parse({ pattern: "TODO", path: "/tmp", include: "*.ts" })
const parsed = parse(Grep, { pattern: "TODO", path: "/tmp", include: "*.ts" })
expect(parsed.path).toBe("/tmp")
expect(parsed.include).toBe("*.ts")
})
test("rejects missing pattern", () => {
expect(Grep.safeParse({}).success).toBe(false)
expect(accepts(Grep, {})).toBe(false)
})
})
describe("invalid", () => {
test("accepts tool + error", () => {
expect(Invalid.parse({ tool: "foo", error: "bar" })).toEqual({ tool: "foo", error: "bar" })
expect(parse(Invalid, { tool: "foo", error: "bar" })).toEqual({ tool: "foo", error: "bar" })
})
test("rejects missing fields", () => {
expect(Invalid.safeParse({ tool: "foo" }).success).toBe(false)
expect(Invalid.safeParse({ error: "bar" }).success).toBe(false)
expect(accepts(Invalid, { tool: "foo" })).toBe(false)
expect(accepts(Invalid, { error: "bar" })).toBe(false)
})
})
describe("lsp", () => {
test("accepts all fields", () => {
const parsed = Lsp.parse({ operation: "hover", filePath: "/a.ts", line: 1, character: 1 })
const parsed = parse(Lsp, { operation: "hover", filePath: "/a.ts", line: 1, character: 1 })
expect(parsed.operation).toBe("hover")
})
test("rejects line < 1", () => {
expect(Lsp.safeParse({ operation: "hover", filePath: "/a.ts", line: 0, character: 1 }).success).toBe(false)
expect(accepts(Lsp, { operation: "hover", filePath: "/a.ts", line: 0, character: 1 })).toBe(false)
})
test("rejects character < 1", () => {
expect(Lsp.safeParse({ operation: "hover", filePath: "/a.ts", line: 1, character: 0 }).success).toBe(false)
expect(accepts(Lsp, { operation: "hover", filePath: "/a.ts", line: 1, character: 0 })).toBe(false)
})
test("rejects unknown operation", () => {
expect(Lsp.safeParse({ operation: "bogus", filePath: "/a.ts", line: 1, character: 1 }).success).toBe(false)
expect(accepts(Lsp, { operation: "bogus", filePath: "/a.ts", line: 1, character: 1 })).toBe(false)
})
})
describe("multiedit", () => {
test("accepts empty edits array", () => {
expect(MultiEdit.parse({ filePath: "/a", edits: [] }).edits).toEqual([])
expect(parse(MultiEdit, { filePath: "/a", edits: [] }).edits).toEqual([])
})
test("accepts an edit entry", () => {
const parsed = MultiEdit.parse({
const parsed = parse(MultiEdit, {
filePath: "/a",
edits: [{ filePath: "/a", oldString: "x", newString: "y" }],
})
@@ -184,13 +187,13 @@ describe("tool parameters", () => {
describe("plan", () => {
test("accepts empty object", () => {
expect(Plan.parse({})).toEqual({})
expect(parse(Plan, {})).toEqual({})
})
})
describe("question", () => {
test("accepts questions array", () => {
const parsed = Question.parse({
const parsed = parse(Question, {
questions: [
{
question: "pick one",
@@ -203,16 +206,16 @@ describe("tool parameters", () => {
expect(parsed.questions.length).toBe(1)
})
test("rejects missing questions", () => {
expect(Question.safeParse({}).success).toBe(false)
expect(accepts(Question, {})).toBe(false)
})
})
describe("read", () => {
test("accepts filePath-only", () => {
expect(Read.parse({ filePath: "/a" }).filePath).toBe("/a")
expect(parse(Read, { filePath: "/a" }).filePath).toBe("/a")
})
test("accepts optional offset + limit", () => {
const parsed = Read.parse({ filePath: "/a", offset: 10, limit: 100 })
const parsed = parse(Read, { filePath: "/a", offset: 10, limit: 100 })
expect(parsed.offset).toBe(10)
expect(parsed.limit).toBe(100)
})
@@ -220,53 +223,53 @@ describe("tool parameters", () => {
describe("skill", () => {
test("accepts name", () => {
expect(Skill.parse({ name: "foo" }).name).toBe("foo")
expect(parse(Skill, { name: "foo" }).name).toBe("foo")
})
test("rejects missing name", () => {
expect(Skill.safeParse({}).success).toBe(false)
expect(accepts(Skill, {})).toBe(false)
})
})
describe("task", () => {
test("accepts description + prompt + subagent_type", () => {
const parsed = Task.parse({ description: "d", prompt: "p", subagent_type: "general" })
const parsed = parse(Task, { description: "d", prompt: "p", subagent_type: "general" })
expect(parsed.subagent_type).toBe("general")
})
test("rejects missing prompt", () => {
expect(Task.safeParse({ description: "d", subagent_type: "general" }).success).toBe(false)
expect(accepts(Task, { description: "d", subagent_type: "general" })).toBe(false)
})
})
describe("todo", () => {
test("accepts todos array", () => {
const parsed = Todo.parse({
const parsed = parse(Todo, {
todos: [{ id: "t1", content: "do x", status: "pending", priority: "medium" }],
})
expect(parsed.todos.length).toBe(1)
})
test("rejects missing todos", () => {
expect(Todo.safeParse({}).success).toBe(false)
expect(accepts(Todo, {})).toBe(false)
})
})
describe("webfetch", () => {
test("accepts url-only", () => {
expect(WebFetch.parse({ url: "https://example.com" }).url).toBe("https://example.com")
expect(parse(WebFetch, { url: "https://example.com" }).url).toBe("https://example.com")
})
})
describe("websearch", () => {
test("accepts query", () => {
expect(WebSearch.parse({ query: "opencode" }).query).toBe("opencode")
expect(parse(WebSearch, { query: "opencode" }).query).toBe("opencode")
})
})
describe("write", () => {
test("accepts content + filePath", () => {
expect(Write.parse({ content: "hi", filePath: "/a" })).toEqual({ content: "hi", filePath: "/a" })
expect(parse(Write, { content: "hi", filePath: "/a" })).toEqual({ content: "hi", filePath: "/a" })
})
test("rejects missing filePath", () => {
expect(Write.safeParse({ content: "hi" }).success).toBe(false)
expect(accepts(Write, { content: "hi" })).toBe(false)
})
})
})
@@ -1,13 +1,13 @@
import { describe, test, expect } from "bun:test"
import { Effect, Layer, ManagedRuntime } from "effect"
import z from "zod"
import { Effect, Layer, ManagedRuntime, Schema } from "effect"
import { Agent } from "../../src/agent/agent"
import { MessageID, SessionID } from "../../src/session/schema"
import { Tool } from "../../src/tool"
import { Truncate } from "../../src/tool"
const runtime = ManagedRuntime.make(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer))
const params = z.object({ input: z.string() })
const params = Schema.Struct({ input: Schema.String })
function makeTool(id: string, executeFn?: () => void) {
return {
@@ -56,4 +56,44 @@ describe("Tool.define", () => {
expect(first).not.toBe(second)
})
test("execute receives decoded parameters", async () => {
const parameters = Schema.Struct({
count: Schema.NumberFromString.pipe(Schema.optional, Schema.withDecodingDefaultType(Effect.succeed(5))),
})
const calls: Array<Schema.Schema.Type<typeof parameters>> = []
const info = await runtime.runPromise(
Tool.define(
"test-decoded",
Effect.succeed({
description: "test tool",
parameters,
execute(args: Schema.Schema.Type<typeof parameters>) {
calls.push(args)
return Effect.succeed({ title: "test", output: "ok", metadata: { truncated: false } })
},
}),
),
)
const ctx: Tool.Context = {
sessionID: SessionID.descending(),
messageID: MessageID.ascending(),
agent: "build",
abort: new AbortController().signal,
messages: [],
metadata() {
return Effect.void
},
ask() {
return Effect.void
},
}
const tool = await Effect.runPromise(info.init())
const execute = tool.execute as unknown as (args: unknown, ctx: Tool.Context) => ReturnType<typeof tool.execute>
await Effect.runPromise(execute({}, ctx))
await Effect.runPromise(execute({ count: "7" }, ctx))
expect(calls).toEqual([{ count: 5 }, { count: 7 }])
})
})