From b139bc2ef3de411ca0487817fc71958fba12b315 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Fri, 10 Apr 2026 16:57:12 -0400 Subject: [PATCH] refactor(tool): convert write tool to Tool.defineEffect (#21901) --- packages/opencode/src/tool/registry.ts | 3 +- packages/opencode/src/tool/write.ts | 143 ++++---- packages/opencode/test/tool/write.test.ts | 426 ++++++++-------------- 3 files changed, 239 insertions(+), 333 deletions(-) diff --git a/packages/opencode/src/tool/registry.ts b/packages/opencode/src/tool/registry.ts index 7f566ecd00..bd27802ff8 100644 --- a/packages/opencode/src/tool/registry.ts +++ b/packages/opencode/src/tool/registry.ts @@ -110,6 +110,7 @@ export namespace ToolRegistry { const bash = yield* BashTool const codesearch = yield* CodeSearchTool const globtool = yield* GlobTool + const writetool = yield* WriteTool const state = yield* InstanceState.make( Effect.fn("ToolRegistry.state")(function* (ctx) { @@ -173,7 +174,7 @@ export namespace ToolRegistry { glob: Tool.init(globtool), grep: Tool.init(GrepTool), edit: Tool.init(EditTool), - write: Tool.init(WriteTool), + write: Tool.init(writetool), task: Tool.init(task), fetch: Tool.init(webfetch), todo: Tool.init(todo), diff --git a/packages/opencode/src/tool/write.ts b/packages/opencode/src/tool/write.ts index 6b134e5253..444ff6ea66 100644 --- a/packages/opencode/src/tool/write.ts +++ b/packages/opencode/src/tool/write.ts @@ -1,5 +1,6 @@ import z from "zod" import * as path from "path" +import { Effect } from "effect" import { Tool } from "./tool" import { LSP } from "../lsp" import { createTwoFilesPatch } from "diff" @@ -9,76 +10,90 @@ import { File } from "../file" import { FileWatcher } from "../file/watcher" import { Format } from "../format" import { FileTime } from "../file/time" -import { Filesystem } from "../util/filesystem" +import { AppFileSystem } from "../filesystem" import { Instance } from "../project/instance" import { trimDiff } from "./edit" -import { assertExternalDirectory } from "./external-directory" +import { assertExternalDirectoryEffect } from "./external-directory" const MAX_DIAGNOSTICS_PER_FILE = 20 const MAX_PROJECT_DIAGNOSTICS_FILES = 5 -export const WriteTool = Tool.define("write", { - description: DESCRIPTION, - parameters: z.object({ - content: z.string().describe("The content to write to the file"), - filePath: z.string().describe("The absolute path to the file to write (must be absolute, not relative)"), - }), - async execute(params, ctx) { - const filepath = path.isAbsolute(params.filePath) ? params.filePath : path.join(Instance.directory, params.filePath) - await assertExternalDirectory(ctx, filepath) - - const exists = await Filesystem.exists(filepath) - const contentOld = exists ? await Filesystem.readText(filepath) : "" - if (exists) await FileTime.assert(ctx.sessionID, filepath) - - const diff = trimDiff(createTwoFilesPatch(filepath, filepath, contentOld, params.content)) - await ctx.ask({ - permission: "edit", - patterns: [path.relative(Instance.worktree, filepath)], - always: ["*"], - metadata: { - filepath, - diff, - }, - }) - - await Filesystem.write(filepath, params.content) - await Format.file(filepath) - Bus.publish(File.Event.Edited, { file: filepath }) - await Bus.publish(FileWatcher.Event.Updated, { - file: filepath, - event: exists ? "change" : "add", - }) - await FileTime.read(ctx.sessionID, filepath) - - let output = "Wrote file successfully." - await LSP.touchFile(filepath, true) - const diagnostics = await LSP.diagnostics() - const normalizedFilepath = Filesystem.normalizePath(filepath) - let projectDiagnosticsCount = 0 - for (const [file, issues] of Object.entries(diagnostics)) { - const errors = issues.filter((item) => item.severity === 1) - if (errors.length === 0) continue - const limited = errors.slice(0, MAX_DIAGNOSTICS_PER_FILE) - const suffix = - errors.length > MAX_DIAGNOSTICS_PER_FILE ? `\n... and ${errors.length - MAX_DIAGNOSTICS_PER_FILE} more` : "" - if (file === normalizedFilepath) { - output += `\n\nLSP errors detected in this file, please fix:\n\n${limited.map(LSP.Diagnostic.pretty).join("\n")}${suffix}\n` - continue - } - if (projectDiagnosticsCount >= MAX_PROJECT_DIAGNOSTICS_FILES) continue - projectDiagnosticsCount++ - output += `\n\nLSP errors detected in other files:\n\n${limited.map(LSP.Diagnostic.pretty).join("\n")}${suffix}\n` - } +export const WriteTool = Tool.defineEffect( + "write", + Effect.gen(function* () { + const lsp = yield* LSP.Service + const fs = yield* AppFileSystem.Service + const filetime = yield* FileTime.Service return { - title: path.relative(Instance.worktree, filepath), - metadata: { - diagnostics, - filepath, - exists: exists, - }, - output, + description: DESCRIPTION, + parameters: z.object({ + content: z.string().describe("The content to write to the file"), + filePath: z.string().describe("The absolute path to the file to write (must be absolute, not relative)"), + }), + execute: (params: { content: string; filePath: string }, ctx: Tool.Context) => + Effect.gen(function* () { + const filepath = path.isAbsolute(params.filePath) ? params.filePath : path.join(Instance.directory, params.filePath) + yield* assertExternalDirectoryEffect(ctx, filepath) + + const exists = yield* fs.existsSafe(filepath) + const contentOld = exists ? yield* fs.readFileString(filepath) : "" + if (exists) yield* filetime.assert(ctx.sessionID, filepath) + + const diff = trimDiff(createTwoFilesPatch(filepath, filepath, contentOld, params.content)) + yield* Effect.promise(() => + ctx.ask({ + permission: "edit", + patterns: [path.relative(Instance.worktree, filepath)], + always: ["*"], + metadata: { + filepath, + diff, + }, + }), + ) + + yield* fs.writeWithDirs(filepath, params.content) + yield* Effect.promise(() => Format.file(filepath)) + Bus.publish(File.Event.Edited, { file: filepath }) + yield* Effect.promise(() => + Bus.publish(FileWatcher.Event.Updated, { + file: filepath, + event: exists ? "change" : "add", + }), + ) + yield* filetime.read(ctx.sessionID, filepath) + + let output = "Wrote file successfully." + yield* lsp.touchFile(filepath, true) + const diagnostics = yield* lsp.diagnostics() + const normalizedFilepath = AppFileSystem.normalizePath(filepath) + let projectDiagnosticsCount = 0 + for (const [file, issues] of Object.entries(diagnostics)) { + const errors = issues.filter((item) => item.severity === 1) + if (errors.length === 0) continue + const limited = errors.slice(0, MAX_DIAGNOSTICS_PER_FILE) + const suffix = + errors.length > MAX_DIAGNOSTICS_PER_FILE ? `\n... and ${errors.length - MAX_DIAGNOSTICS_PER_FILE} more` : "" + if (file === normalizedFilepath) { + output += `\n\nLSP errors detected in this file, please fix:\n\n${limited.map(LSP.Diagnostic.pretty).join("\n")}${suffix}\n` + continue + } + if (projectDiagnosticsCount >= MAX_PROJECT_DIAGNOSTICS_FILES) continue + projectDiagnosticsCount++ + output += `\n\nLSP errors detected in other files:\n\n${limited.map(LSP.Diagnostic.pretty).join("\n")}${suffix}\n` + } + + return { + title: path.relative(Instance.worktree, filepath), + metadata: { + diagnostics, + filepath, + exists: exists, + }, + output, + } + }).pipe(Effect.orDie, Effect.runPromise), } - }, -}) + }), +) diff --git a/packages/opencode/test/tool/write.test.ts b/packages/opencode/test/tool/write.test.ts index 97939c1051..8289646ebe 100644 --- a/packages/opencode/test/tool/write.test.ts +++ b/packages/opencode/test/tool/write.test.ts @@ -1,10 +1,17 @@ -import { afterEach, describe, test, expect } from "bun:test" +import { afterEach, describe, expect } from "bun:test" +import { Effect, Layer } from "effect" import path from "path" import fs from "fs/promises" import { WriteTool } from "../../src/tool/write" import { Instance } from "../../src/project/instance" -import { tmpdir } from "../fixture/fixture" +import { LSP } from "../../src/lsp" +import { AppFileSystem } from "../../src/filesystem" +import { FileTime } from "../../src/file/time" +import { Tool } from "../../src/tool/tool" import { SessionID, MessageID } from "../../src/session/schema" +import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" +import { provideTmpdirInstance } from "../fixture/fixture" +import { testEffect } from "../lib/effect" const ctx = { sessionID: SessionID.make("ses_test-write-session"), @@ -21,333 +28,216 @@ afterEach(async () => { await Instance.disposeAll() }) +const it = testEffect( + Layer.mergeAll(LSP.defaultLayer, AppFileSystem.defaultLayer, FileTime.defaultLayer, CrossSpawnSpawner.defaultLayer), +) + +const init = Effect.fn("WriteToolTest.init")(function* () { + const info = yield* WriteTool + return yield* Effect.promise(() => info.init()) +}) + +const run = Effect.fn("WriteToolTest.run")(function* ( + args: Tool.InferParameters, + next: Tool.Context = ctx, +) { + const tool = yield* init() + return yield* Effect.promise(() => tool.execute(args, next)) +}) + +const markRead = Effect.fn("WriteToolTest.markRead")(function* (sessionID: string, filepath: string) { + const ft = yield* FileTime.Service + yield* ft.read(sessionID as any, filepath) +}) + describe("tool.write", () => { describe("new file creation", () => { - test("writes content to new file", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "newfile.txt") - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - const result = await write.execute( - { - filePath: filepath, - content: "Hello, World!", - }, - ctx, - ) + it.live("writes content to new file", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "newfile.txt") + const result = yield* run({ filePath: filepath, content: "Hello, World!" }) expect(result.output).toContain("Wrote file successfully") expect(result.metadata.exists).toBe(false) - const content = await fs.readFile(filepath, "utf-8") + const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8")) expect(content).toBe("Hello, World!") - }, - }) - }) + }), + ), + ) - test("creates parent directories if needed", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "nested", "deep", "file.txt") + it.live("creates parent directories if needed", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "nested", "deep", "file.txt") + yield* run({ filePath: filepath, content: "nested content" }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - await write.execute( - { - filePath: filepath, - content: "nested content", - }, - ctx, - ) - - const content = await fs.readFile(filepath, "utf-8") + const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8")) expect(content).toBe("nested content") - }, - }) - }) + }), + ), + ) - test("handles relative paths by resolving to instance directory", async () => { - await using tmp = await tmpdir() + it.live("handles relative paths by resolving to instance directory", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + yield* run({ filePath: "relative.txt", content: "relative content" }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - await write.execute( - { - filePath: "relative.txt", - content: "relative content", - }, - ctx, - ) - - const content = await fs.readFile(path.join(tmp.path, "relative.txt"), "utf-8") + const content = yield* Effect.promise(() => fs.readFile(path.join(dir, "relative.txt"), "utf-8")) expect(content).toBe("relative content") - }, - }) - }) + }), + ), + ) }) describe("existing file overwrite", () => { - test("overwrites existing file content", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "existing.txt") - await fs.writeFile(filepath, "old content", "utf-8") + it.live("overwrites existing file content", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "existing.txt") + yield* Effect.promise(() => fs.writeFile(filepath, "old content", "utf-8")) + yield* markRead(ctx.sessionID, filepath) - // First read the file to satisfy FileTime requirement - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const { FileTime } = await import("../../src/file/time") - await FileTime.read(ctx.sessionID, filepath) - - const write = await WriteTool.init() - const result = await write.execute( - { - filePath: filepath, - content: "new content", - }, - ctx, - ) + const result = yield* run({ filePath: filepath, content: "new content" }) expect(result.output).toContain("Wrote file successfully") expect(result.metadata.exists).toBe(true) - const content = await fs.readFile(filepath, "utf-8") + const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8")) expect(content).toBe("new content") - }, - }) - }) + }), + ), + ) - test("returns diff in metadata for existing files", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "file.txt") - await fs.writeFile(filepath, "old", "utf-8") + it.live("returns diff in metadata for existing files", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "file.txt") + yield* Effect.promise(() => fs.writeFile(filepath, "old", "utf-8")) + yield* markRead(ctx.sessionID, filepath) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const { FileTime } = await import("../../src/file/time") - await FileTime.read(ctx.sessionID, filepath) + const result = yield* run({ filePath: filepath, content: "new" }) - const write = await WriteTool.init() - const result = await write.execute( - { - filePath: filepath, - content: "new", - }, - ctx, - ) - - // Diff should be in metadata expect(result.metadata).toHaveProperty("filepath", filepath) expect(result.metadata).toHaveProperty("exists", true) - }, - }) - }) + }), + ), + ) }) describe("file permissions", () => { - test("sets file permissions when writing sensitive data", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "sensitive.json") + it.live("sets file permissions when writing sensitive data", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "sensitive.json") + yield* run({ filePath: filepath, content: JSON.stringify({ secret: "data" }) }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - await write.execute( - { - filePath: filepath, - content: JSON.stringify({ secret: "data" }), - }, - ctx, - ) - - // On Unix systems, check permissions if (process.platform !== "win32") { - const stats = await fs.stat(filepath) + const stats = yield* Effect.promise(() => fs.stat(filepath)) expect(stats.mode & 0o777).toBe(0o644) } - }, - }) - }) + }), + ), + ) }) describe("content types", () => { - test("writes JSON content", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "data.json") - const data = { key: "value", nested: { array: [1, 2, 3] } } + it.live("writes JSON content", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "data.json") + const data = { key: "value", nested: { array: [1, 2, 3] } } + yield* run({ filePath: filepath, content: JSON.stringify(data, null, 2) }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - await write.execute( - { - filePath: filepath, - content: JSON.stringify(data, null, 2), - }, - ctx, - ) - - const content = await fs.readFile(filepath, "utf-8") + const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8")) expect(JSON.parse(content)).toEqual(data) - }, - }) - }) + }), + ), + ) - test("writes binary-safe content", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "binary.bin") - const content = "Hello\x00World\x01\x02\x03" + it.live("writes binary-safe content", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "binary.bin") + const content = "Hello\x00World\x01\x02\x03" + yield* run({ filePath: filepath, content }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - await write.execute( - { - filePath: filepath, - content, - }, - ctx, - ) - - const buf = await fs.readFile(filepath) + const buf = yield* Effect.promise(() => fs.readFile(filepath)) expect(buf.toString()).toBe(content) - }, - }) - }) + }), + ), + ) - test("writes empty content", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "empty.txt") + it.live("writes empty content", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "empty.txt") + yield* run({ filePath: filepath, content: "" }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - await write.execute( - { - filePath: filepath, - content: "", - }, - ctx, - ) - - const content = await fs.readFile(filepath, "utf-8") + const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8")) expect(content).toBe("") - const stats = await fs.stat(filepath) + const stats = yield* Effect.promise(() => fs.stat(filepath)) expect(stats.size).toBe(0) - }, - }) - }) + }), + ), + ) - test("writes multi-line content", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "multiline.txt") - const lines = ["Line 1", "Line 2", "Line 3", ""].join("\n") + it.live("writes multi-line content", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "multiline.txt") + const lines = ["Line 1", "Line 2", "Line 3", ""].join("\n") + yield* run({ filePath: filepath, content: lines }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - await write.execute( - { - filePath: filepath, - content: lines, - }, - ctx, - ) - - const content = await fs.readFile(filepath, "utf-8") + const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8")) expect(content).toBe(lines) - }, - }) - }) + }), + ), + ) - test("handles different line endings", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "crlf.txt") - const content = "Line 1\r\nLine 2\r\nLine 3" + it.live("handles different line endings", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "crlf.txt") + const content = "Line 1\r\nLine 2\r\nLine 3" + yield* run({ filePath: filepath, content }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - await write.execute( - { - filePath: filepath, - content, - }, - ctx, - ) - - const buf = await fs.readFile(filepath) + const buf = yield* Effect.promise(() => fs.readFile(filepath)) expect(buf.toString()).toBe(content) - }, - }) - }) + }), + ), + ) }) describe("error handling", () => { - test("throws error when OS denies write access", async () => { - await using tmp = await tmpdir() - const readonlyPath = path.join(tmp.path, "readonly.txt") + it.live("throws error when OS denies write access", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const readonlyPath = path.join(dir, "readonly.txt") + yield* Effect.promise(() => fs.writeFile(readonlyPath, "test", "utf-8")) + yield* Effect.promise(() => fs.chmod(readonlyPath, 0o444)) + yield* markRead(ctx.sessionID, readonlyPath) - // Create a read-only file - await fs.writeFile(readonlyPath, "test", "utf-8") - await fs.chmod(readonlyPath, 0o444) - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const { FileTime } = await import("../../src/file/time") - await FileTime.read(ctx.sessionID, readonlyPath) - - const write = await WriteTool.init() - await expect( - write.execute( - { - filePath: readonlyPath, - content: "new content", - }, - ctx, - ), - ).rejects.toThrow() - }, - }) - }) + const exit = yield* run({ filePath: readonlyPath, content: "new content" }).pipe(Effect.exit) + expect(exit._tag).toBe("Failure") + }), + ), + ) }) describe("title generation", () => { - test("returns relative path as title", async () => { - await using tmp = await tmpdir() - const filepath = path.join(tmp.path, "src", "components", "Button.tsx") - await fs.mkdir(path.dirname(filepath), { recursive: true }) - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const write = await WriteTool.init() - const result = await write.execute( - { - filePath: filepath, - content: "export const Button = () => {}", - }, - ctx, - ) + it.live("returns relative path as title", () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "src", "components", "Button.tsx") + yield* Effect.promise(() => fs.mkdir(path.dirname(filepath), { recursive: true })) + const result = yield* run({ filePath: filepath, content: "export const Button = () => {}" }) expect(result.title).toEndWith(path.join("src", "components", "Button.tsx")) - }, - }) - }) + }), + ), + ) }) })