diff --git a/src/index.ts b/src/index.ts index 1daef01..9f2f012 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,4 +1,4 @@ -import { mkdir, readFile, realpath, rm, stat, writeFile } from "node:fs/promises"; +import { mkdir, readFile, realpath, rename, rm, stat, unlink, writeFile } from "node:fs/promises"; import path from "node:path"; import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import type { Model } from "@mariozechner/pi-ai"; @@ -66,8 +66,47 @@ type ApplyPatchPreview = { type ApplyPatchToolDetails = { preview?: ApplyPatchPreview; + result?: ApplyPatchResult; }; +export type ApplyPatchFailure = { + filePath: string; + operation: ApplyPatchOperation; + message: string; +}; + +export type ApplyPatchRecoveryInstructions = { + mustReadFiles: string[]; + mustNotReadFiles: string[]; +}; + +export type ApplyPatchResult = { + summaries: string[]; + appliedFiles: string[]; + failures: ApplyPatchFailure[]; + hasPartialSuccess: boolean; + recoveryInstructions: ApplyPatchRecoveryInstructions; + details: { + fuzz: number; + }; +}; + +export class ApplyPatchError extends Error { + public readonly failures: ApplyPatchFailure[]; + public readonly result: ApplyPatchResult; + + constructor(message: string, result: ApplyPatchResult) { + super(message); + this.name = "ApplyPatchError"; + this.failures = result.failures; + this.result = result; + } + + hasPartialSuccess(): boolean { + return this.result.hasPartialSuccess; + } +} + type ApplyPatchThemeColor = | "accent" | "error" @@ -85,6 +124,48 @@ type ApplyPatchTheme = { inverse: (text: string) => string; }; +type AtomicWriteOperations = { + writeFile: (filePath: string, content: string, encoding: "utf-8") => Promise; + rename: (fromPath: string, toPath: string) => Promise; + unlink: (filePath: string) => Promise; +}; + +const ATOMIC_WRITE_OPERATIONS: AtomicWriteOperations = { + writeFile, + rename, + unlink, +}; + +function hasErrorCode(error: unknown, code: string): boolean { + return Boolean(error && typeof error === "object" && "code" in error && error.code === code); +} + +async function writeFileAtomic( + absPath: string, + content: string, + operations: AtomicWriteOperations = ATOMIC_WRITE_OPERATIONS, +): Promise { + const tempPath = `${absPath}.tmp.${process.pid}.${Math.random().toString(16).slice(2)}`; + await operations.writeFile(tempPath, content, "utf-8"); + try { + await operations.rename(tempPath, absPath); + } catch (error) { + if (!hasErrorCode(error, "EEXIST")) { + throw error; + } + await operations.unlink(absPath); + await operations.rename(tempPath, absPath); + } +} + +export async function __testWriteFileAtomic( + absPath: string, + content: string, + operations: AtomicWriteOperations, +): Promise { + await writeFileAtomic(absPath, content, operations); +} + const GPT_APPLY_PATCH_PROVIDERS = new Set(["openai", "azure-openai-responses", "github-copilot"]); function normalizeApplyPatchArguments(args: unknown): ApplyPatchParams { @@ -154,9 +235,14 @@ function normalizeSeekLine(line: string): string { .replace(/[\u00A0\u2002-\u200A\u202F\u205F\u3000]/g, " "); } -function seekSequence(lines: string[], pattern: string[], start: number, eof: boolean): number | undefined { +function seekSequence( + lines: string[], + pattern: string[], + start: number, + eof: boolean, +): { index: number; fuzz: 0 | 1 | 100 | 10000 } | undefined { if (pattern.length === 0) { - return start; + return { index: start, fuzz: 0 }; } if (pattern.length > lines.length) { return undefined; @@ -177,22 +263,22 @@ function seekSequence(lines: string[], pattern: string[], start: number, eof: bo for (let index = searchStart; index <= lastStart; index++) { if (matches(index, (line, expected) => line === expected)) { - return index; + return { index, fuzz: 0 }; } } for (let index = searchStart; index <= lastStart; index++) { if (matches(index, (line, expected) => line.trimEnd() === expected.trimEnd())) { - return index; + return { index, fuzz: 1 }; } } for (let index = searchStart; index <= lastStart; index++) { if (matches(index, (line, expected) => line.trim() === expected.trim())) { - return index; + return { index, fuzz: 100 }; } } for (let index = searchStart; index <= lastStart; index++) { if (matches(index, (line, expected) => normalizeSeekLine(line) === normalizeSeekLine(expected))) { - return index; + return { index, fuzz: 10000 }; } } @@ -350,7 +436,8 @@ async function createPatchPreview(cwd: string, hunks: ParsedPatch[]): Promise { - const summaries: string[] = []; +async function applySingleHunk( + cwd: string, + hunk: ParsedPatch, +): Promise<{ summary: string; appliedFile: string; fuzz: number }> { + const absolutePath = await resolveWorkspacePath(cwd, hunk.filePath); + if (hunk.type === "add") { + await mkdir(path.dirname(absolutePath), { recursive: true }); + await assertWorkspacePath(cwd, absolutePath); + await writeFileAtomic(absolutePath, hunk.content); + return { summary: `add: ${hunk.filePath}`, appliedFile: hunk.filePath, fuzz: 0 }; + } - for (const hunk of hunks) { - const absolutePath = await resolveWorkspacePath(cwd, hunk.filePath); - if (hunk.type === "add") { - await mkdir(path.dirname(absolutePath), { recursive: true }); - await assertWorkspacePath(cwd, absolutePath); - await writeFile(absolutePath, hunk.content, "utf-8"); - summaries.push(`add: ${hunk.filePath}`); - continue; - } + if (hunk.type === "delete") { + await stat(absolutePath); + await assertWorkspacePath(cwd, absolutePath); + await rm(absolutePath); + return { summary: `delete: ${hunk.filePath}`, appliedFile: hunk.filePath, fuzz: 0 }; + } - if (hunk.type === "delete") { - await stat(absolutePath); - await assertWorkspacePath(cwd, absolutePath); + const currentContent = await readFile(absolutePath, "utf-8"); + const chunkResult = + hunk.chunks.length === 0 + ? { content: currentContent, fuzz: 0 } + : replaceChunks(currentContent, hunk.filePath, hunk.chunks); + const nextContent = chunkResult.content; + + if (hunk.movePath) { + const absoluteMovePath = await resolveWorkspacePath(cwd, hunk.movePath); + await mkdir(path.dirname(absoluteMovePath), { recursive: true }); + await assertWorkspacePath(cwd, absoluteMovePath); + await writeFileAtomic(absoluteMovePath, nextContent); + if (absoluteMovePath !== absolutePath) { await rm(absolutePath); - summaries.push(`delete: ${hunk.filePath}`); - continue; } + return { + summary: `move: ${hunk.filePath} -> ${hunk.movePath}`, + appliedFile: hunk.movePath, + fuzz: chunkResult.fuzz, + }; + } - const currentContent = await readFile(absolutePath, "utf-8"); - const nextContent = - hunk.chunks.length === 0 ? currentContent : replaceChunks(currentContent, hunk.filePath, hunk.chunks); + await assertWorkspacePath(cwd, absolutePath); + await writeFileAtomic(absolutePath, nextContent); + return { summary: `update: ${hunk.filePath}`, appliedFile: hunk.filePath, fuzz: chunkResult.fuzz }; +} - if (hunk.movePath) { - const absoluteMovePath = await resolveWorkspacePath(cwd, hunk.movePath); - await mkdir(path.dirname(absoluteMovePath), { recursive: true }); - await assertWorkspacePath(cwd, absoluteMovePath); - await writeFile(absoluteMovePath, nextContent, "utf-8"); - if (absoluteMovePath !== absolutePath) { - await rm(absolutePath); - } - summaries.push(`move: ${hunk.filePath} -> ${hunk.movePath}`); - continue; +export async function applyPatchDetailed(cwd: string, patchText: string): Promise { + const hunks = parsePatch(patchText); + if (hunks.length === 0) { + const normalized = normalizePatchText(patchText).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"); + } - await assertWorkspacePath(cwd, absolutePath); - await writeFile(absolutePath, nextContent, "utf-8"); - summaries.push(`update: ${hunk.filePath}`); + const summaries: string[] = []; + const appliedFiles: string[] = []; + const failures: ApplyPatchFailure[] = []; + let fuzz = 0; + + for (const hunk of hunks) { + try { + const { summary, appliedFile, fuzz: hunkFuzz } = await applySingleHunk(cwd, hunk); + summaries.push(summary); + appliedFiles.push(appliedFile); + fuzz += hunkFuzz; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + failures.push({ filePath: hunk.filePath, operation: hunk.type, message }); + } } - return summaries; + const result: ApplyPatchResult = { + summaries, + appliedFiles, + failures, + hasPartialSuccess: appliedFiles.length > 0 && failures.length > 0, + recoveryInstructions: { mustReadFiles: [], mustNotReadFiles: [] }, + details: { fuzz }, + }; + result.recoveryInstructions = createRecoveryInstructions(result); + return result; +} + +function createRecoveryInstructions( + result: Pick, +): ApplyPatchRecoveryInstructions { + const mustReadFiles = [...new Set(result.failures.map((failure) => failure.filePath))]; + const mustNotReadFiles = [...new Set(result.appliedFiles.filter((filePath) => !mustReadFiles.includes(filePath)))]; + return { mustReadFiles, mustNotReadFiles }; } export async function applyPatch(cwd: string, patchText: string): Promise { @@ -626,7 +764,31 @@ export async function applyPatch(cwd: string, patchText: string): Promise 0, + recoveryInstructions: createRecoveryInstructions({ + appliedFiles, + failures: [{ filePath: hunk.filePath, operation: hunk.type, message }], + }), + details: { fuzz: 0 }, + }; + throw new ApplyPatchError(message, result); + } + } + + return summaries; } async function createPendingPatchUpdate( @@ -772,10 +934,36 @@ export function createApplyPatchTool(): ApplyPatchToolDefinition { details: pendingUpdate.details, }); - const summaries = await applyPatch(ctx.cwd, normalizedParams.input); + const result = await applyPatchDetailed(ctx.cwd, normalizedParams.input); + if (result.failures.length > 0) { + const failed = result.recoveryInstructions.mustReadFiles.join(", "); + const mustReadText = failed.includes(",") ? failed.split(", ").join(" and ") : failed; + return { + content: [ + { + type: "text", + text: [ + "apply_patch partially failed.", + `Failed: ${failed}`, + `Recovery: MUST read ${mustReadText} before retrying.`, + result.appliedFiles.length > 0 + ? "Earlier file actions in this patch were already applied." + : "No file actions were applied.", + result.recoveryInstructions.mustNotReadFiles.length > 0 + ? "Recovery: MUST NOT reread other files from this patch unless a specific dependency requires it." + : "", + ] + .filter((line) => line.length > 0) + .join("\n"), + }, + ], + details: { result }, + }; + } + return { - content: [{ type: "text", text: summaries.join("\n") }], - details: {}, + content: [{ type: "text", text: result.summaries.join("\n") }], + details: { result }, }; }, renderCall(_args, theme) { diff --git a/test/index.test.ts b/test/index.test.ts index 4c28a42..74481bb 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -1,11 +1,13 @@ -import { mkdtemp, readFile, rm, symlink, writeFile } from "node:fs/promises"; +import { mkdtemp, readdir, readFile, rm, symlink, writeFile } from "node:fs/promises"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { + __testWriteFileAtomic, APPLY_PATCH_FREEFORM_DESCRIPTION, APPLY_PATCH_LARK_GRAMMAR, type ApplyPatchExtensionAPI, applyPatch, + applyPatchDetailed, createApplyPatchTool, extractPatchedPaths, type FreeformToolFormat, @@ -412,6 +414,159 @@ EOF`; await expect(applyPatch(directory, patch)).rejects.toThrow("Failed to find expected lines in modify.txt"); }); + it("#given partial patch failure #when applying detailed #then accumulates applied and failed files", async () => { + // given + const directory = await createTempDirectory(); + await writeFile(path.join(directory, "ok.txt"), "before\n", "utf-8"); + await writeFile(path.join(directory, "broken.txt"), "line\n", "utf-8"); + const patch = `*** Begin Patch +*** Update File: ok.txt +@@ +-before ++after +*** Update File: broken.txt +@@ +-missing ++changed +*** End Patch`; + + // when + const result = await applyPatchDetailed(directory, patch); + + // then + expect(result.appliedFiles).toEqual(["ok.txt"]); + expect(result.failures).toHaveLength(1); + expect(result.failures[0]?.filePath).toBe("broken.txt"); + expect(result.recoveryInstructions.mustReadFiles).toEqual(["broken.txt"]); + expect(result.recoveryInstructions.mustNotReadFiles).toEqual(["ok.txt"]); + }); + + it("#given partial patch failure #when applying compat api #then fails fast after first error", async () => { + // given + const directory = await createTempDirectory(); + await writeFile(path.join(directory, "broken.txt"), "line\n", "utf-8"); + await writeFile(path.join(directory, "later.txt"), "before\n", "utf-8"); + const patch = `*** Begin Patch +*** Update File: broken.txt +@@ +-missing ++changed +*** Update File: later.txt +@@ +-before ++after +*** End Patch`; + + // when / then + await expect(applyPatch(directory, patch)).rejects.toThrow("Failed to find expected lines in broken.txt"); + expect(await readFile(path.join(directory, "later.txt"), "utf-8")).toBe("before\n"); + }); + + it("#given fuzzy matches across hunks #when applying detailed #then aggregates fuzz score", async () => { + // given + const directory = await createTempDirectory(); + await writeFile(path.join(directory, "trim-end.txt"), "keep trailing \n", "utf-8"); + await writeFile(path.join(directory, "normalize.txt"), "name = “old”\n", "utf-8"); + const patch = `*** Begin Patch +*** Update File: trim-end.txt +@@ +-keep trailing ++keep trailing updated +*** Update File: normalize.txt +@@ +-name = "old" ++name = "new" +*** End Patch`; + + // when + const result = await applyPatchDetailed(directory, patch); + + // then + expect(result.failures).toEqual([]); + expect(result.details.fuzz).toBe(10001); + }); + + it("#given apply patch tool partial failure #when executed #then returns recovery instructions text", async () => { + // given + const directory = await createTempDirectory(); + await writeFile(path.join(directory, "ok.txt"), "before\n", "utf-8"); + await writeFile(path.join(directory, "broken.txt"), "line\n", "utf-8"); + const patch = `*** Begin Patch +*** Update File: ok.txt +@@ +-before ++after +*** Update File: broken.txt +@@ +-missing ++changed +*** End Patch`; + + // when + const result = await createApplyPatchTool().execute("apply-patch-test", { input: patch }, undefined, undefined, { + cwd: directory, + } as never); + + // then + const text = result.content.find((block) => block.type === "text")?.text ?? ""; + expect(text).toContain("apply_patch partially failed."); + expect(text).toContain("Failed: broken.txt"); + expect(text).toContain("Recovery: MUST read broken.txt before retrying."); + expect(text).toContain("Earlier file actions in this patch were already applied."); + expect(text).toContain( + "Recovery: MUST NOT reread other files from this patch unless a specific dependency requires it.", + ); + }); + + it("#given successful patch write #when applying patch #then atomic temp files are cleaned", async () => { + // given + const directory = await createTempDirectory(); + await writeFile(path.join(directory, "atomic.txt"), "before\n", "utf-8"); + const patch = `*** Begin Patch +*** Update File: atomic.txt +@@ +-before ++after +*** End Patch`; + + // when + await applyPatch(directory, patch); + + // then + expect(await readFile(path.join(directory, "atomic.txt"), "utf-8")).toBe("after\n"); + const files = await readdir(directory); + expect(files.some((name) => name.includes(".tmp."))).toBe(false); + }); + + it("#given eexist on rename #when writing atomically #then retries after unlink", async () => { + // given + const calls: string[] = []; + let renameCount = 0; + const operations = { + async writeFile() { + calls.push("writeFile"); + }, + async rename() { + renameCount += 1; + calls.push(`rename:${renameCount}`); + if (renameCount === 1) { + const error = new Error("exists") as Error & { code?: string }; + error.code = "EEXIST"; + throw error; + } + }, + async unlink() { + calls.push("unlink"); + }, + }; + + // when + await __testWriteFileAtomic("/tmp/target.txt", "content", operations); + + // then + expect(calls).toEqual(["writeFile", "rename:1", "unlink", "rename:2"]); + }); + it("#given patch text #when extracting paths #then returns touched files", () => { // given const patch = `*** Begin Patch