refactor(tool): convert apply_patch to Tool.defineEffect (#21938)

This commit is contained in:
Kit Langton
2026-04-10 19:42:14 -04:00
committed by GitHub
parent d9d5a0615e
commit 0556774097
5 changed files with 264 additions and 230 deletions
+247 -228
View File
@@ -1,16 +1,16 @@
import z from "zod" import z from "zod"
import * as path from "path" import * as path from "path"
import * as fs from "fs/promises" import { Effect } from "effect"
import { Tool } from "./tool" import { Tool } from "./tool"
import { Bus } from "../bus" import { Bus } from "../bus"
import { FileWatcher } from "../file/watcher" import { FileWatcher } from "../file/watcher"
import { Instance } from "../project/instance" import { Instance } from "../project/instance"
import { Patch } from "../patch" import { Patch } from "../patch"
import { createTwoFilesPatch, diffLines } from "diff" import { createTwoFilesPatch, diffLines } from "diff"
import { assertExternalDirectory } from "./external-directory" import { assertExternalDirectoryEffect } from "./external-directory"
import { trimDiff } from "./edit" import { trimDiff } from "./edit"
import { LSP } from "../lsp" import { LSP } from "../lsp"
import { Filesystem } from "../util/filesystem" import { AppFileSystem } from "../filesystem"
import DESCRIPTION from "./apply_patch.txt" import DESCRIPTION from "./apply_patch.txt"
import { File } from "../file" import { File } from "../file"
import { Format } from "../format" 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"), patchText: z.string().describe("The full patch text that describes all changes to be made"),
}) })
export const ApplyPatchTool = Tool.define("apply_patch", { export const ApplyPatchTool = Tool.defineEffect(
description: DESCRIPTION, "apply_patch",
parameters: PatchParams, Effect.gen(function* () {
async execute(params, ctx) { const lsp = yield* LSP.Service
if (!params.patchText) { const afs = yield* AppFileSystem.Service
throw new Error("patchText is required") const format = yield* Format.Service
}
// Parse the patch to get hunks const run = Effect.fn("ApplyPatchTool.execute")(function* (params: z.infer<typeof PatchParams>, ctx: Tool.Context) {
let hunks: Patch.Hunk[] if (!params.patchText) {
try { return yield* Effect.fail(new Error("patchText is required"))
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")
} }
throw new Error("apply_patch verification failed: no hunks found")
}
// Validate file paths and check permissions // Parse the patch to get hunks
const fileChanges: Array<{ let hunks: Patch.Hunk[]
filePath: string try {
oldContent: string const parseResult = Patch.parsePatch(params.patchText)
newContent: string hunks = parseResult.hunks
type: "add" | "update" | "delete" | "move" } catch (error) {
movePath?: string return yield* Effect.fail(new Error(`apply_patch verification failed: ${error}`))
diff: string }
additions: number
deletions: number
}> = []
let totalDiff = "" if (hunks.length === 0) {
const normalized = params.patchText.replace(/\r\n/g, "\n").replace(/\r/g, "\n").trim()
for (const hunk of hunks) { if (normalized === "*** Begin Patch\n*** End Patch") {
const filePath = path.resolve(Instance.directory, hunk.path) return yield* Effect.fail(new Error("patch rejected: empty patch"))
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
} }
return yield* Effect.fail(new Error("apply_patch verification failed: no hunks found"))
}
case "update": { // Validate file paths and check permissions
// Check if file exists for update const fileChanges: Array<{
const stats = await fs.stat(filePath).catch(() => null) filePath: string
if (!stats || stats.isDirectory()) { oldContent: string
throw new Error(`apply_patch verification failed: Failed to read file to update: ${filePath}`) 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") case "update": {
let newContent = oldContent // 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 const oldContent = yield* afs.readFileString(filePath)
try { let newContent = oldContent
const fileUpdate = Patch.deriveNewContentsFromChunks(filePath, hunk.chunks)
newContent = fileUpdate.content // Apply the update chunks to get new content
} catch (error) { try {
throw new Error(`apply_patch verification failed: ${error}`) 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 const deletions = contentToDelete.split("\n").length
let deletions = 0
for (const change of diffLines(oldContent, newContent)) { fileChanges.push({
if (change.added) additions += change.count || 0 filePath,
if (change.removed) deletions += change.count || 0 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) // Build per-file metadata for UI rendering (used for both permission and result)
const files = fileChanges.map((change) => ({ const files = fileChanges.map((change) => ({
filePath: change.filePath, filePath: change.filePath,
relativePath: path.relative(Instance.worktree, change.movePath ?? change.filePath).replaceAll("\\", "/"), relativePath: path.relative(Instance.worktree, change.movePath ?? change.filePath).replaceAll("\\", "/"),
type: change.type, type: change.type,
patch: change.diff, patch: change.diff,
additions: change.additions, additions: change.additions,
deletions: change.deletions, deletions: change.deletions,
movePath: change.movePath, movePath: change.movePath,
})) }))
// Check permissions if needed // Check permissions if needed
const relativePaths = fileChanges.map((c) => path.relative(Instance.worktree, c.filePath).replaceAll("\\", "/")) const relativePaths = fileChanges.map((c) => path.relative(Instance.worktree, c.filePath).replaceAll("\\", "/"))
await ctx.ask({ yield* Effect.promise(() =>
permission: "edit", ctx.ask({
patterns: relativePaths, permission: "edit",
always: ["*"], patterns: relativePaths,
metadata: { always: ["*"],
filepath: relativePaths.join(", "), metadata: {
diff: totalDiff, filepath: relativePaths.join(", "),
files, diff: totalDiff,
}, files,
}) },
}),
)
// Apply the changes // Apply the changes
const updates: Array<{ file: string; event: "add" | "change" | "unlink" }> = [] const updates: Array<{ file: string; event: "add" | "change" | "unlink" }> = []
for (const change of fileChanges) { for (const change of fileChanges) {
const edited = change.type === "delete" ? undefined : (change.movePath ?? change.filePath) const edited = change.type === "delete" ? undefined : (change.movePath ?? change.filePath)
switch (change.type) { switch (change.type) {
case "add": 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) {
// Create parent directories (recursive: true is safe on existing/root dirs) // 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") yield* afs.writeWithDirs(change.filePath, change.newContent)
await fs.unlink(change.filePath) 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.filePath, event: "unlink" })
updates.push({ file: change.movePath, event: "add" }) break
} }
break
case "delete": if (edited) {
await fs.unlink(change.filePath) yield* format.file(edited)
updates.push({ file: change.filePath, event: "unlink" }) Bus.publish(File.Event.Edited, { file: edited })
break }
} }
if (edited) { // Publish file change events
await Format.file(edited) for (const update of updates) {
Bus.publish(File.Event.Edited, { file: edited }) Bus.publish(FileWatcher.Event.Updated, update)
} }
}
// Publish file change events // Notify LSP of file changes and collect diagnostics
for (const update of updates) { for (const change of fileChanges) {
await Bus.publish(FileWatcher.Event.Updated, update) if (change.type === "delete") continue
} const target = change.movePath ?? change.filePath
yield* lsp.touchFile(target, true)
// 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("\\", "/")}`
} }
if (change.type === "delete") { const diagnostics = yield* lsp.diagnostics()
return `D ${path.relative(Instance.worktree, change.filePath).replaceAll("\\", "/")}`
// 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<diagnostics file="${target}">\n${limited.map(LSP.Diagnostic.pretty).join("\n")}${suffix}\n</diagnostics>`
}
}
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<diagnostics file="${target}">\n${limited.map(LSP.Diagnostic.pretty).join("\n")}${suffix}\n</diagnostics>`
}
}
return { return {
title: output, description: DESCRIPTION,
metadata: { parameters: PatchParams,
diff: totalDiff, async execute(params: z.infer<typeof PatchParams>, ctx) {
files, return Effect.runPromise(run(params, ctx).pipe(Effect.orDie))
diagnostics,
}, },
output,
} }
}, }),
}) )
+5 -1
View File
@@ -34,6 +34,7 @@ import { FetchHttpClient, HttpClient } from "effect/unstable/http"
import { ChildProcessSpawner } from "effect/unstable/process/ChildProcessSpawner" import { ChildProcessSpawner } from "effect/unstable/process/ChildProcessSpawner"
import * as CrossSpawnSpawner from "@/effect/cross-spawn-spawner" import * as CrossSpawnSpawner from "@/effect/cross-spawn-spawner"
import { Ripgrep } from "../file/ripgrep" import { Ripgrep } from "../file/ripgrep"
import { Format } from "../format"
import { InstanceState } from "@/effect/instance-state" 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"
@@ -91,6 +92,7 @@ export namespace ToolRegistry {
| HttpClient.HttpClient | HttpClient.HttpClient
| ChildProcessSpawner | ChildProcessSpawner
| Ripgrep.Service | Ripgrep.Service
| Format.Service
> = Layer.effect( > = Layer.effect(
Service, Service,
Effect.gen(function* () { Effect.gen(function* () {
@@ -113,6 +115,7 @@ export namespace ToolRegistry {
const writetool = yield* WriteTool const writetool = yield* WriteTool
const edit = yield* EditTool const edit = yield* EditTool
const greptool = yield* GrepTool const greptool = yield* GrepTool
const patchtool = yield* ApplyPatchTool
const state = yield* InstanceState.make<State>( const state = yield* InstanceState.make<State>(
Effect.fn("ToolRegistry.state")(function* (ctx) { Effect.fn("ToolRegistry.state")(function* (ctx) {
@@ -183,7 +186,7 @@ export namespace ToolRegistry {
search: Tool.init(websearch), search: Tool.init(websearch),
code: Tool.init(codesearch), code: Tool.init(codesearch),
skill: Tool.init(SkillTool), skill: Tool.init(SkillTool),
patch: Tool.init(ApplyPatchTool), patch: Tool.init(patchtool),
question: Tool.init(question), question: Tool.init(question),
lsp: Tool.init(lsptool), lsp: Tool.init(lsptool),
plan: Tool.init(plan), plan: Tool.init(plan),
@@ -325,6 +328,7 @@ export namespace ToolRegistry {
Layer.provide(Instruction.defaultLayer), Layer.provide(Instruction.defaultLayer),
Layer.provide(AppFileSystem.defaultLayer), Layer.provide(AppFileSystem.defaultLayer),
Layer.provide(FetchHttpClient.layer), Layer.provide(FetchHttpClient.layer),
Layer.provide(Format.defaultLayer),
Layer.provide(CrossSpawnSpawner.defaultLayer), Layer.provide(CrossSpawnSpawner.defaultLayer),
Layer.provide(Ripgrep.defaultLayer), Layer.provide(Ripgrep.defaultLayer),
), ),
@@ -38,6 +38,7 @@ import { Truncate } from "../../src/tool/truncate"
import { Log } from "../../src/util/log" import { Log } from "../../src/util/log"
import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner"
import { Ripgrep } from "../../src/file/ripgrep" import { Ripgrep } from "../../src/file/ripgrep"
import { Format } from "../../src/format"
import { provideTmpdirInstance, provideTmpdirServer } from "../fixture/fixture" import { provideTmpdirInstance, provideTmpdirServer } from "../fixture/fixture"
import { testEffect } from "../lib/effect" import { testEffect } from "../lib/effect"
import { reply, TestLLMServer } from "../lib/llm-server" import { reply, TestLLMServer } from "../lib/llm-server"
@@ -174,6 +175,7 @@ function makeHttp() {
Layer.provide(FetchHttpClient.layer), Layer.provide(FetchHttpClient.layer),
Layer.provide(CrossSpawnSpawner.defaultLayer), Layer.provide(CrossSpawnSpawner.defaultLayer),
Layer.provide(Ripgrep.defaultLayer), Layer.provide(Ripgrep.defaultLayer),
Layer.provide(Format.defaultLayer),
Layer.provideMerge(todo), Layer.provideMerge(todo),
Layer.provideMerge(question), Layer.provideMerge(question),
Layer.provideMerge(deps), Layer.provideMerge(deps),
@@ -54,6 +54,7 @@ import { Truncate } from "../../src/tool/truncate"
import { AppFileSystem } from "../../src/filesystem" import { AppFileSystem } from "../../src/filesystem"
import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner"
import { Ripgrep } from "../../src/file/ripgrep" import { Ripgrep } from "../../src/file/ripgrep"
import { Format } from "../../src/format"
Log.init({ print: false }) Log.init({ print: false })
@@ -138,6 +139,7 @@ function makeHttp() {
Layer.provide(FetchHttpClient.layer), Layer.provide(FetchHttpClient.layer),
Layer.provide(CrossSpawnSpawner.defaultLayer), Layer.provide(CrossSpawnSpawner.defaultLayer),
Layer.provide(Ripgrep.defaultLayer), Layer.provide(Ripgrep.defaultLayer),
Layer.provide(Format.defaultLayer),
Layer.provideMerge(todo), Layer.provideMerge(todo),
Layer.provideMerge(question), Layer.provideMerge(question),
Layer.provideMerge(deps), Layer.provideMerge(deps),
@@ -1,11 +1,17 @@
import { describe, expect, test } from "bun:test" import { describe, expect, test } from "bun:test"
import path from "path" import path from "path"
import * as fs from "fs/promises" import * as fs from "fs/promises"
import { Effect, ManagedRuntime, Layer } from "effect"
import { ApplyPatchTool } from "../../src/tool/apply_patch" import { ApplyPatchTool } from "../../src/tool/apply_patch"
import { Instance } from "../../src/project/instance" 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 { tmpdir } from "../fixture/fixture"
import { SessionID, MessageID } from "../../src/session/schema" import { SessionID, MessageID } from "../../src/session/schema"
const runtime = ManagedRuntime.make(Layer.mergeAll(LSP.defaultLayer, AppFileSystem.defaultLayer, Format.defaultLayer))
const baseCtx = { const baseCtx = {
sessionID: SessionID.make("ses_test"), sessionID: SessionID.make("ses_test"),
messageID: MessageID.make(""), messageID: MessageID.make(""),
@@ -40,7 +46,8 @@ type ToolCtx = typeof baseCtx & {
} }
const execute = async (params: { patchText: string }, ctx: ToolCtx) => { 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) return tool.execute(params, ctx)
} }