From 0556774097b0223b1e547260d2b7f46640c6e884 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Fri, 10 Apr 2026 19:42:14 -0400 Subject: [PATCH] refactor(tool): convert apply_patch to Tool.defineEffect (#21938) --- packages/opencode/src/tool/apply_patch.ts | 475 +++++++++--------- packages/opencode/src/tool/registry.ts | 6 +- .../test/session/prompt-effect.test.ts | 2 + .../test/session/snapshot-tool-race.test.ts | 2 + .../opencode/test/tool/apply_patch.test.ts | 9 +- 5 files changed, 264 insertions(+), 230 deletions(-) diff --git a/packages/opencode/src/tool/apply_patch.ts b/packages/opencode/src/tool/apply_patch.ts index 30b2e91ace..9645fc1ed2 100644 --- a/packages/opencode/src/tool/apply_patch.ts +++ b/packages/opencode/src/tool/apply_patch.ts @@ -1,16 +1,16 @@ import z from "zod" import * as path from "path" -import * as fs from "fs/promises" +import { Effect } from "effect" import { Tool } from "./tool" import { Bus } from "../bus" import { FileWatcher } from "../file/watcher" import { Instance } from "../project/instance" import { Patch } from "../patch" import { createTwoFilesPatch, diffLines } from "diff" -import { assertExternalDirectory } from "./external-directory" +import { assertExternalDirectoryEffect } from "./external-directory" import { trimDiff } from "./edit" import { LSP } from "../lsp" -import { Filesystem } from "../util/filesystem" +import { AppFileSystem } from "../filesystem" import DESCRIPTION from "./apply_patch.txt" import { File } from "../file" import { Format } from "../format" @@ -19,261 +19,280 @@ const PatchParams = z.object({ patchText: z.string().describe("The full patch text that describes all changes to be made"), }) -export const ApplyPatchTool = Tool.define("apply_patch", { - description: DESCRIPTION, - parameters: PatchParams, - async execute(params, ctx) { - if (!params.patchText) { - throw new Error("patchText is required") - } +export const ApplyPatchTool = Tool.defineEffect( + "apply_patch", + Effect.gen(function* () { + const lsp = yield* LSP.Service + const afs = yield* AppFileSystem.Service + const format = yield* Format.Service - // Parse the patch to get hunks - let hunks: Patch.Hunk[] - try { - const parseResult = Patch.parsePatch(params.patchText) - hunks = parseResult.hunks - } catch (error) { - throw new Error(`apply_patch verification failed: ${error}`) - } - - if (hunks.length === 0) { - const normalized = params.patchText.replace(/\r\n/g, "\n").replace(/\r/g, "\n").trim() - if (normalized === "*** Begin Patch\n*** End Patch") { - throw new Error("patch rejected: empty patch") + const run = Effect.fn("ApplyPatchTool.execute")(function* (params: z.infer, ctx: Tool.Context) { + if (!params.patchText) { + return yield* Effect.fail(new Error("patchText is required")) } - throw new Error("apply_patch verification failed: no hunks found") - } - // Validate file paths and check permissions - const fileChanges: Array<{ - filePath: string - oldContent: string - newContent: string - type: "add" | "update" | "delete" | "move" - movePath?: string - diff: string - additions: number - deletions: number - }> = [] + // Parse the patch to get hunks + let hunks: Patch.Hunk[] + try { + const parseResult = Patch.parsePatch(params.patchText) + hunks = parseResult.hunks + } catch (error) { + return yield* Effect.fail(new Error(`apply_patch verification failed: ${error}`)) + } - let totalDiff = "" - - for (const hunk of hunks) { - const filePath = path.resolve(Instance.directory, hunk.path) - await assertExternalDirectory(ctx, filePath) - - switch (hunk.type) { - case "add": { - const oldContent = "" - const newContent = - hunk.contents.length === 0 || hunk.contents.endsWith("\n") ? hunk.contents : `${hunk.contents}\n` - const diff = trimDiff(createTwoFilesPatch(filePath, filePath, oldContent, newContent)) - - let additions = 0 - let deletions = 0 - for (const change of diffLines(oldContent, newContent)) { - if (change.added) additions += change.count || 0 - if (change.removed) deletions += change.count || 0 - } - - fileChanges.push({ - filePath, - oldContent, - newContent, - type: "add", - diff, - additions, - deletions, - }) - - totalDiff += diff + "\n" - break + if (hunks.length === 0) { + const normalized = params.patchText.replace(/\r\n/g, "\n").replace(/\r/g, "\n").trim() + if (normalized === "*** Begin Patch\n*** End Patch") { + return yield* Effect.fail(new Error("patch rejected: empty patch")) } + return yield* Effect.fail(new Error("apply_patch verification failed: no hunks found")) + } - case "update": { - // Check if file exists for update - const stats = await fs.stat(filePath).catch(() => null) - if (!stats || stats.isDirectory()) { - throw new Error(`apply_patch verification failed: Failed to read file to update: ${filePath}`) + // Validate file paths and check permissions + const fileChanges: Array<{ + filePath: string + oldContent: string + newContent: string + type: "add" | "update" | "delete" | "move" + movePath?: string + diff: string + additions: number + deletions: number + }> = [] + + let totalDiff = "" + + for (const hunk of hunks) { + const filePath = path.resolve(Instance.directory, hunk.path) + yield* assertExternalDirectoryEffect(ctx, filePath) + + switch (hunk.type) { + case "add": { + const oldContent = "" + const newContent = + hunk.contents.length === 0 || hunk.contents.endsWith("\n") ? hunk.contents : `${hunk.contents}\n` + const diff = trimDiff(createTwoFilesPatch(filePath, filePath, oldContent, newContent)) + + let additions = 0 + let deletions = 0 + for (const change of diffLines(oldContent, newContent)) { + if (change.added) additions += change.count || 0 + if (change.removed) deletions += change.count || 0 + } + + fileChanges.push({ + filePath, + oldContent, + newContent, + type: "add", + diff, + additions, + deletions, + }) + + totalDiff += diff + "\n" + break } - const oldContent = await fs.readFile(filePath, "utf-8") - let newContent = oldContent + case "update": { + // Check if file exists for update + const stats = yield* afs.stat(filePath).pipe(Effect.catch(() => Effect.succeed(undefined))) + if (!stats || stats.type === "Directory") { + return yield* Effect.fail( + new Error(`apply_patch verification failed: Failed to read file to update: ${filePath}`), + ) + } - // Apply the update chunks to get new content - try { - const fileUpdate = Patch.deriveNewContentsFromChunks(filePath, hunk.chunks) - newContent = fileUpdate.content - } catch (error) { - throw new Error(`apply_patch verification failed: ${error}`) + const oldContent = yield* afs.readFileString(filePath) + let newContent = oldContent + + // Apply the update chunks to get new content + try { + const fileUpdate = Patch.deriveNewContentsFromChunks(filePath, hunk.chunks) + newContent = fileUpdate.content + } catch (error) { + return yield* Effect.fail(new Error(`apply_patch verification failed: ${error}`)) + } + + const diff = trimDiff(createTwoFilesPatch(filePath, filePath, oldContent, newContent)) + + let additions = 0 + let deletions = 0 + for (const change of diffLines(oldContent, newContent)) { + if (change.added) additions += change.count || 0 + if (change.removed) deletions += change.count || 0 + } + + const movePath = hunk.move_path ? path.resolve(Instance.directory, hunk.move_path) : undefined + yield* assertExternalDirectoryEffect(ctx, movePath) + + fileChanges.push({ + filePath, + oldContent, + newContent, + type: hunk.move_path ? "move" : "update", + movePath, + diff, + additions, + deletions, + }) + + totalDiff += diff + "\n" + break } - const diff = trimDiff(createTwoFilesPatch(filePath, filePath, oldContent, newContent)) + case "delete": { + const contentToDelete = yield* afs.readFileString(filePath).pipe( + Effect.catch((error) => Effect.fail(new Error(`apply_patch verification failed: ${error}`))), + ) + const deleteDiff = trimDiff(createTwoFilesPatch(filePath, filePath, contentToDelete, "")) - let additions = 0 - let deletions = 0 - for (const change of diffLines(oldContent, newContent)) { - if (change.added) additions += change.count || 0 - if (change.removed) deletions += change.count || 0 + const deletions = contentToDelete.split("\n").length + + fileChanges.push({ + filePath, + oldContent: contentToDelete, + newContent: "", + type: "delete", + diff: deleteDiff, + additions: 0, + deletions, + }) + + totalDiff += deleteDiff + "\n" + break } - - const movePath = hunk.move_path ? path.resolve(Instance.directory, hunk.move_path) : undefined - await assertExternalDirectory(ctx, movePath) - - fileChanges.push({ - filePath, - oldContent, - newContent, - type: hunk.move_path ? "move" : "update", - movePath, - diff, - additions, - deletions, - }) - - totalDiff += diff + "\n" - break - } - - case "delete": { - const contentToDelete = await fs.readFile(filePath, "utf-8").catch((error) => { - throw new Error(`apply_patch verification failed: ${error}`) - }) - const deleteDiff = trimDiff(createTwoFilesPatch(filePath, filePath, contentToDelete, "")) - - const deletions = contentToDelete.split("\n").length - - fileChanges.push({ - filePath, - oldContent: contentToDelete, - newContent: "", - type: "delete", - diff: deleteDiff, - additions: 0, - deletions, - }) - - totalDiff += deleteDiff + "\n" - break } } - } - // Build per-file metadata for UI rendering (used for both permission and result) - const files = fileChanges.map((change) => ({ - filePath: change.filePath, - relativePath: path.relative(Instance.worktree, change.movePath ?? change.filePath).replaceAll("\\", "/"), - type: change.type, - patch: change.diff, - additions: change.additions, - deletions: change.deletions, - movePath: change.movePath, - })) + // Build per-file metadata for UI rendering (used for both permission and result) + const files = fileChanges.map((change) => ({ + filePath: change.filePath, + relativePath: path.relative(Instance.worktree, change.movePath ?? change.filePath).replaceAll("\\", "/"), + type: change.type, + patch: change.diff, + additions: change.additions, + deletions: change.deletions, + movePath: change.movePath, + })) - // Check permissions if needed - const relativePaths = fileChanges.map((c) => path.relative(Instance.worktree, c.filePath).replaceAll("\\", "/")) - await ctx.ask({ - permission: "edit", - patterns: relativePaths, - always: ["*"], - metadata: { - filepath: relativePaths.join(", "), - diff: totalDiff, - files, - }, - }) + // Check permissions if needed + const relativePaths = fileChanges.map((c) => path.relative(Instance.worktree, c.filePath).replaceAll("\\", "/")) + yield* Effect.promise(() => + ctx.ask({ + permission: "edit", + patterns: relativePaths, + always: ["*"], + metadata: { + filepath: relativePaths.join(", "), + diff: totalDiff, + files, + }, + }), + ) - // Apply the changes - const updates: Array<{ file: string; event: "add" | "change" | "unlink" }> = [] + // Apply the changes + const updates: Array<{ file: string; event: "add" | "change" | "unlink" }> = [] - for (const change of fileChanges) { - const edited = change.type === "delete" ? undefined : (change.movePath ?? change.filePath) - switch (change.type) { - case "add": - // Create parent directories (recursive: true is safe on existing/root dirs) - await fs.mkdir(path.dirname(change.filePath), { recursive: true }) - await fs.writeFile(change.filePath, change.newContent, "utf-8") - updates.push({ file: change.filePath, event: "add" }) - break - - case "update": - await fs.writeFile(change.filePath, change.newContent, "utf-8") - updates.push({ file: change.filePath, event: "change" }) - break - - case "move": - if (change.movePath) { + for (const change of fileChanges) { + const edited = change.type === "delete" ? undefined : (change.movePath ?? change.filePath) + switch (change.type) { + case "add": // Create parent directories (recursive: true is safe on existing/root dirs) - await fs.mkdir(path.dirname(change.movePath), { recursive: true }) - await fs.writeFile(change.movePath, change.newContent, "utf-8") - await fs.unlink(change.filePath) + + yield* afs.writeWithDirs(change.filePath, change.newContent) + updates.push({ file: change.filePath, event: "add" }) + break + + case "update": + yield* afs.writeWithDirs(change.filePath, change.newContent) + updates.push({ file: change.filePath, event: "change" }) + break + + case "move": + if (change.movePath) { + // Create parent directories (recursive: true is safe on existing/root dirs) + + yield* afs.writeWithDirs(change.movePath!, change.newContent) + yield* afs.remove(change.filePath) + updates.push({ file: change.filePath, event: "unlink" }) + updates.push({ file: change.movePath, event: "add" }) + } + break + + case "delete": + yield* afs.remove(change.filePath) updates.push({ file: change.filePath, event: "unlink" }) - updates.push({ file: change.movePath, event: "add" }) - } - break + break + } - case "delete": - await fs.unlink(change.filePath) - updates.push({ file: change.filePath, event: "unlink" }) - break + if (edited) { + yield* format.file(edited) + Bus.publish(File.Event.Edited, { file: edited }) + } } - if (edited) { - await Format.file(edited) - Bus.publish(File.Event.Edited, { file: edited }) + // Publish file change events + for (const update of updates) { + Bus.publish(FileWatcher.Event.Updated, update) } - } - // Publish file change events - for (const update of updates) { - await Bus.publish(FileWatcher.Event.Updated, update) - } - - // Notify LSP of file changes and collect diagnostics - for (const change of fileChanges) { - if (change.type === "delete") continue - const target = change.movePath ?? change.filePath - await LSP.touchFile(target, true) - } - const diagnostics = await LSP.diagnostics() - - // Generate output summary - const summaryLines = fileChanges.map((change) => { - if (change.type === "add") { - return `A ${path.relative(Instance.worktree, change.filePath).replaceAll("\\", "/")}` + // Notify LSP of file changes and collect diagnostics + for (const change of fileChanges) { + if (change.type === "delete") continue + const target = change.movePath ?? change.filePath + yield* lsp.touchFile(target, true) } - if (change.type === "delete") { - return `D ${path.relative(Instance.worktree, change.filePath).replaceAll("\\", "/")}` + const diagnostics = yield* lsp.diagnostics() + + // Generate output summary + const summaryLines = fileChanges.map((change) => { + if (change.type === "add") { + return `A ${path.relative(Instance.worktree, change.filePath).replaceAll("\\", "/")}` + } + if (change.type === "delete") { + return `D ${path.relative(Instance.worktree, change.filePath).replaceAll("\\", "/")}` + } + const target = change.movePath ?? change.filePath + return `M ${path.relative(Instance.worktree, target).replaceAll("\\", "/")}` + }) + let output = `Success. Updated the following files:\n${summaryLines.join("\n")}` + + // Report LSP errors for changed files + const MAX_DIAGNOSTICS_PER_FILE = 20 + for (const change of fileChanges) { + if (change.type === "delete") continue + const target = change.movePath ?? change.filePath + const normalized = AppFileSystem.normalizePath(target) + const issues = diagnostics[normalized] ?? [] + const errors = issues.filter((item) => item.severity === 1) + if (errors.length > 0) { + 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` + : "" + output += `\n\nLSP errors detected in ${path.relative(Instance.worktree, target).replaceAll("\\", "/")}, please fix:\n\n${limited.map(LSP.Diagnostic.pretty).join("\n")}${suffix}\n` + } + } + + return { + title: output, + metadata: { + diff: totalDiff, + files, + diagnostics, + }, + output, } - const target = change.movePath ?? change.filePath - return `M ${path.relative(Instance.worktree, target).replaceAll("\\", "/")}` }) - let output = `Success. Updated the following files:\n${summaryLines.join("\n")}` - - // Report LSP errors for changed files - const MAX_DIAGNOSTICS_PER_FILE = 20 - for (const change of fileChanges) { - if (change.type === "delete") continue - const target = change.movePath ?? change.filePath - const normalized = Filesystem.normalizePath(target) - const issues = diagnostics[normalized] ?? [] - const errors = issues.filter((item) => item.severity === 1) - if (errors.length > 0) { - 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` : "" - output += `\n\nLSP errors detected in ${path.relative(Instance.worktree, target).replaceAll("\\", "/")}, please fix:\n\n${limited.map(LSP.Diagnostic.pretty).join("\n")}${suffix}\n` - } - } return { - title: output, - metadata: { - diff: totalDiff, - files, - diagnostics, + description: DESCRIPTION, + parameters: PatchParams, + async execute(params: z.infer, ctx) { + return Effect.runPromise(run(params, ctx).pipe(Effect.orDie)) }, - output, } - }, -}) + }), +) diff --git a/packages/opencode/src/tool/registry.ts b/packages/opencode/src/tool/registry.ts index 84dd7b79aa..a24ddb28c4 100644 --- a/packages/opencode/src/tool/registry.ts +++ b/packages/opencode/src/tool/registry.ts @@ -34,6 +34,7 @@ import { FetchHttpClient, HttpClient } from "effect/unstable/http" import { ChildProcessSpawner } from "effect/unstable/process/ChildProcessSpawner" import * as CrossSpawnSpawner from "@/effect/cross-spawn-spawner" import { Ripgrep } from "../file/ripgrep" +import { Format } from "../format" import { InstanceState } from "@/effect/instance-state" import { makeRuntime } from "@/effect/run-service" import { Env } from "../env" @@ -91,6 +92,7 @@ export namespace ToolRegistry { | HttpClient.HttpClient | ChildProcessSpawner | Ripgrep.Service + | Format.Service > = Layer.effect( Service, Effect.gen(function* () { @@ -113,6 +115,7 @@ export namespace ToolRegistry { const writetool = yield* WriteTool const edit = yield* EditTool const greptool = yield* GrepTool + const patchtool = yield* ApplyPatchTool const state = yield* InstanceState.make( Effect.fn("ToolRegistry.state")(function* (ctx) { @@ -183,7 +186,7 @@ export namespace ToolRegistry { search: Tool.init(websearch), code: Tool.init(codesearch), skill: Tool.init(SkillTool), - patch: Tool.init(ApplyPatchTool), + patch: Tool.init(patchtool), question: Tool.init(question), lsp: Tool.init(lsptool), plan: Tool.init(plan), @@ -325,6 +328,7 @@ export namespace ToolRegistry { Layer.provide(Instruction.defaultLayer), Layer.provide(AppFileSystem.defaultLayer), Layer.provide(FetchHttpClient.layer), + Layer.provide(Format.defaultLayer), Layer.provide(CrossSpawnSpawner.defaultLayer), Layer.provide(Ripgrep.defaultLayer), ), diff --git a/packages/opencode/test/session/prompt-effect.test.ts b/packages/opencode/test/session/prompt-effect.test.ts index aef88b2334..bd7548d2ad 100644 --- a/packages/opencode/test/session/prompt-effect.test.ts +++ b/packages/opencode/test/session/prompt-effect.test.ts @@ -38,6 +38,7 @@ import { Truncate } from "../../src/tool/truncate" import { Log } from "../../src/util/log" import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" import { Ripgrep } from "../../src/file/ripgrep" +import { Format } from "../../src/format" import { provideTmpdirInstance, provideTmpdirServer } from "../fixture/fixture" import { testEffect } from "../lib/effect" import { reply, TestLLMServer } from "../lib/llm-server" @@ -174,6 +175,7 @@ function makeHttp() { Layer.provide(FetchHttpClient.layer), Layer.provide(CrossSpawnSpawner.defaultLayer), Layer.provide(Ripgrep.defaultLayer), + Layer.provide(Format.defaultLayer), Layer.provideMerge(todo), Layer.provideMerge(question), Layer.provideMerge(deps), diff --git a/packages/opencode/test/session/snapshot-tool-race.test.ts b/packages/opencode/test/session/snapshot-tool-race.test.ts index 10d4d8f6f6..4901c6f4f1 100644 --- a/packages/opencode/test/session/snapshot-tool-race.test.ts +++ b/packages/opencode/test/session/snapshot-tool-race.test.ts @@ -54,6 +54,7 @@ import { Truncate } from "../../src/tool/truncate" import { AppFileSystem } from "../../src/filesystem" import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" import { Ripgrep } from "../../src/file/ripgrep" +import { Format } from "../../src/format" Log.init({ print: false }) @@ -138,6 +139,7 @@ function makeHttp() { Layer.provide(FetchHttpClient.layer), Layer.provide(CrossSpawnSpawner.defaultLayer), Layer.provide(Ripgrep.defaultLayer), + Layer.provide(Format.defaultLayer), Layer.provideMerge(todo), Layer.provideMerge(question), Layer.provideMerge(deps), diff --git a/packages/opencode/test/tool/apply_patch.test.ts b/packages/opencode/test/tool/apply_patch.test.ts index 19c8cfefd0..d54d34b834 100644 --- a/packages/opencode/test/tool/apply_patch.test.ts +++ b/packages/opencode/test/tool/apply_patch.test.ts @@ -1,11 +1,17 @@ import { describe, expect, test } from "bun:test" import path from "path" import * as fs from "fs/promises" +import { Effect, ManagedRuntime, Layer } from "effect" import { ApplyPatchTool } from "../../src/tool/apply_patch" import { Instance } from "../../src/project/instance" +import { LSP } from "../../src/lsp" +import { AppFileSystem } from "../../src/filesystem" +import { Format } from "../../src/format" import { tmpdir } from "../fixture/fixture" import { SessionID, MessageID } from "../../src/session/schema" +const runtime = ManagedRuntime.make(Layer.mergeAll(LSP.defaultLayer, AppFileSystem.defaultLayer, Format.defaultLayer)) + const baseCtx = { sessionID: SessionID.make("ses_test"), messageID: MessageID.make(""), @@ -40,7 +46,8 @@ type ToolCtx = typeof baseCtx & { } const execute = async (params: { patchText: string }, ctx: ToolCtx) => { - const tool = await ApplyPatchTool.init() + const info = await runtime.runPromise(ApplyPatchTool) + const tool = await info.init() return tool.execute(params, ctx) }