Merge pull request #3 from ConsultingFuture4200/fix/apply-patch-truncation

fix(agent): stop apply_patch truncating large files and half-applying patches

Author: voidstackloop

app/src/agent-tools.test.ts

@@ -387,6 +387,88 @@ describe("agent-tools", () => {
             const patch = ["--- /dev/null", "+++ b/../../etc/evil.txt", "@@ -0,0 +1,1 @@", "+pwned", ""].join("\n");
             expect(() => applyPatch(workspace, patch)).toThrow(/outside the workspace/);
         });
+
+        // The patch base used to come from readFile(), which caps its result
+        // at MAX_READ_CHARS and appends a "[truncated ...]" marker — so
+        // patching a file past that cap silently destroyed its tail and wrote
+        // the marker into the source.
+        it("preserves content past the read-truncation cap", () => {
+            const body = Array.from({ length: 12_000 }, (_, i) => `line ${i}`).join("\n");
+            const original = `first\n${body}\n`;
+            expect(original.length).toBeGreaterThan(100_000);
+            fs.writeFileSync(path.join(workspace, "big.txt"), original);
+
+            const patch = ["--- a/big.txt", "+++ b/big.txt", "@@ -1,1 +1,1 @@", "-first", "+FIRST", ""].join("\n");
+            applyPatch(workspace, patch);
+
+            const updated = fs.readFileSync(path.join(workspace, "big.txt"), "utf-8");
+            expect(updated).toBe(`FIRST\n${body}\n`);
+            expect(updated).not.toContain("truncated");
+        });
+
+        // Writing as we went left earlier files modified when a later file in
+        // the same patch failed to align.
+        it("leaves every file untouched when one file in the patch fails to apply", () => {
+            fs.writeFileSync(path.join(workspace, "first.txt"), "alpha\n");
+            fs.writeFileSync(path.join(workspace, "second.txt"), "actual content\n");
+            const patch = [
+                "--- a/first.txt",
+                "+++ b/first.txt",
+                "@@ -1,1 +1,1 @@",
+                "-alpha",
+                "+ALPHA",
+                "--- a/second.txt",
+                "+++ b/second.txt",
+                "@@ -1,1 +1,1 @@",
+                "-expected content",
+                "+new content",
+                "",
+            ].join("\n");
+
+            expect(() => applyPatch(workspace, patch)).toThrow(/Context mismatch/);
+            expect(fs.readFileSync(path.join(workspace, "first.txt"), "utf-8")).toBe("alpha\n");
+            expect(fs.readFileSync(path.join(workspace, "second.txt"), "utf-8")).toBe("actual content\n");
+        });
+
+        // Resolving every file before writing must not mean every file is read
+        // from disk first: two sections for one path have to chain, or the
+        // second silently discards the first.
+        it("chains two sections that touch the same file", () => {
+            fs.writeFileSync(path.join(workspace, "twice.txt"), "one\ntwo\n");
+            const patch = [
+                "--- a/twice.txt",
+                "+++ b/twice.txt",
+                "@@ -1,1 +1,1 @@",
+                "-one",
+                "+ONE",
+                "--- a/twice.txt",
+                "+++ b/twice.txt",
+                "@@ -2,1 +2,1 @@",
+                "-two",
+                "+TWO",
+                "",
+            ].join("\n");
+            const result = applyPatch(workspace, patch);
+            expect(fs.readFileSync(path.join(workspace, "twice.txt"), "utf-8")).toBe("ONE\nTWO\n");
+            expect(result.filesChanged).toEqual(["twice.txt"]);
+        });
+
+        it("patches a file the same diff created", () => {
+            const patch = [
+                "--- /dev/null",
+                "+++ b/made.txt",
+                "@@ -0,0 +1,1 @@",
+                "+hello",
+                "--- a/made.txt",
+                "+++ b/made.txt",
+                "@@ -1,1 +1,1 @@",
+                "-hello",
+                "+HELLO",
+                "",
+            ].join("\n");
+            applyPatch(workspace, patch);
+            expect(fs.readFileSync(path.join(workspace, "made.txt"), "utf-8")).toBe("HELLO");
+        });
     });
 
     describe("executeTool", () => {

app/src/agent-tools.ts

@@ -1235,25 +1235,59 @@ function applyHunksToContent(content: string, hunks: PatchHunk[], filePath: stri
     return result.join("\n");
 }
 
+// Reads a file's complete content for use as the base of an edit. readFile()
+// is display-oriented — it caps its result at MAX_READ_CHARS and appends a
+// "[truncated ...]" marker — which makes it unsafe to write back: everything
+// past the cap would be destroyed and the marker itself saved into the file.
+function readFileForEdit(workspaceRoot: string, relativePath: string): string {
+    const target = resolveSafePath(workspaceRoot, relativePath);
+    if (fs.statSync(target).isDirectory()) throw new Error(`"${relativePath}" is a directory, not a file.`);
+    return fs.readFileSync(target, "utf-8");
+}
+
 export function applyPatch(workspaceRoot: string, patchText: string): { filesChanged: string[] } {
     const files = parseUnifiedDiff(patchText);
     if (files.length === 0) throw new Error("No valid file patches found in the given diff.");
 
-    const filesChanged: string[] = [];
+    // Every file's outcome is resolved in memory before anything is written.
+    // Writing as we went meant a patch whose later file failed to align left
+    // the earlier ones already modified — a half-applied patch, which is the
+    // exact outcome this parser's refusal to fuzzy-match exists to avoid.
+    //
+    // `pending` (rather than a plain list) keeps resolution sequential: a diff
+    // with two sections for the same path, or one that creates a file and then
+    // patches it, has to see the earlier section's result instead of the stale
+    // copy on disk. null means the path is pending deletion.
+    const pending = new Map<string, string | null>();
+    const order: string[] = [];
+    const remember = (relativePath: string, content: string | null): void => {
+        if (!pending.has(relativePath)) order.push(relativePath);
+        pending.set(relativePath, content);
+    };
+
     for (const file of files) {
         if (file.newPath === null) {
             if (!file.oldPath) throw new Error("Patch deletes a file but its path (/dev/null on both sides) is missing.");
-            deletePath(workspaceRoot, file.oldPath, false);
-            filesChanged.push(file.oldPath);
+            remember(file.oldPath, null);
             continue;
         }
-        const isNewFile = file.oldPath === null;
-        const existingContent = isNewFile ? "" : readFile(workspaceRoot, file.newPath);
-        const newContent = applyHunksToContent(existingContent, file.hunks, file.newPath);
-        writeFile(workspaceRoot, file.newPath, newContent);
-        filesChanged.push(file.newPath);
+        let existingContent: string;
+        if (pending.has(file.newPath)) {
+            // Already touched by an earlier section of this same patch — a
+            // pending deletion reads back as empty, i.e. as a fresh file.
+            existingContent = pending.get(file.newPath) ?? "";
+        } else {
+            existingContent = file.oldPath === null ? "" : readFileForEdit(workspaceRoot, file.newPath);
+        }
+        remember(file.newPath, applyHunksToContent(existingContent, file.hunks, file.newPath));
+    }
+
+    for (const relativePath of order) {
+        const content = pending.get(relativePath) ?? null;
+        if (content === null) deletePath(workspaceRoot, relativePath, false);
+        else writeFile(workspaceRoot, relativePath, content);
     }
-    return { filesChanged };
+    return { filesChanged: order };
 }
 
 export interface WebSearchResult {