From a69ff894e4896521503647cfaf07c9930999fff1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Hou=C5=A1ka?= Date: Wed, 4 Nov 2026 22:22:36 +0100 Subject: [PATCH] fix(coding-agent): edit keeps line endings of untouched lines, diffs before writing Edits are spliced into the original text, so lines the edit did not touch keep their CRLF, LF or lone CR endings. The diff and patch are computed before the file is written, so a failure there, such as running out of memory on a large file, leaves the file unchanged. fixes #8544 --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/core/tools/edit-diff.ts | 120 ++++++++++----- packages/coding-agent/src/core/tools/edit.ts | 20 ++- .../test/edit-tool-diff-before-write.test.ts | 30 ++++ packages/coding-agent/test/tools.test.ts | 144 +++++++++++++++++- 5 files changed, 269 insertions(+), 46 deletions(-) create mode 100644 packages/coding-agent/test/edit-tool-diff-before-write.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 223e7f851..094490e25 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -37,6 +37,7 @@ - Fixed OAuth logins such as `openai` and `anthropic` failing with `invalid_grant` after pi was stopped while it refreshed an expired token, for example an RPC client that stops pi right after startup while the background catalog refresh runs. The token endpoint had already replaced the refresh token, but pi exited before saving the new one. On exit, pi now waits for a token refresh in progress to be saved ([#11692](https://github.com/earendil-works/pi/issues/11692)) - Fixed Ctrl+P not switching to the only model in scope (or the only available model) when the active model is a different one; it showed "Only one model in scope" instead. Cycling forward from a model outside the scope now also selects the first scoped model instead of skipping it ([#11806](https://github.com/earendil-works/pi/issues/11806)) - Fixed `/reload` and session switches leaving extension overlays on screen when an extension had more than one `ctx.ui.custom({ overlay: true })` open: only the topmost overlay was removed, and the others kept focus and input although the extension that opened them was gone. All extension overlays, including hidden ones, are now removed and disposed; pi's own overlays stay open +- Fixed the edit tool rewriting the line endings of untouched lines in files with mixed CRLF/LF or lone CR line endings, and leaving a file already written when computing the diff failed, for example by running out of memory on a large file; the diff is now computed before writing ([#8544](https://github.com/earendil-works/pi/issues/8544)) ## [1.5.0] - 2026-11-02 diff --git a/packages/coding-agent/src/core/tools/edit-diff.ts b/packages/coding-agent/src/core/tools/edit-diff.ts index c79f5eb7b..864d403dd 100644 --- a/packages/coding-agent/src/core/tools/edit-diff.ts +++ b/packages/coding-agent/src/core/tools/edit-diff.ts @@ -8,20 +8,63 @@ import { access, readFile } from "fs/promises"; import { splitBom } from "../../utils/text.ts"; import { resolveToCwd } from "./path-utils.ts"; -export function detectLineEnding(content: string): "\r\n" | "\n" { - const crlfIdx = content.indexOf("\r\n"); - const lfIdx = content.indexOf("\n"); - if (lfIdx === -1) return "\n"; - if (crlfIdx === -1) return "\n"; - return crlfIdx < lfIdx ? "\r\n" : "\n"; -} - export function normalizeToLF(text: string): string { return text.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); } -export function restoreLineEndings(text: string, ending: "\r\n" | "\n"): string { - return ending === "\r\n" ? text.replace(/\n/g, "\r\n") : text; +/** + * Apply replacements matched in `normalizeToLF(originalContent)` to `originalContent` itself. + * Text outside the replaced ranges keeps its bytes, including its line endings. Line breaks in + * the new text get the line ending of the line where the replaced range starts. The result, + * normalized to LF, equals the replacements applied to the normalized content. + */ +export function applyReplacementsToOriginal(originalContent: string, replacements: TextReplacement[]): string { + // Only CRLF changes length when normalizing: the LF of the n-th CRLF (at original offset p) is at + // p - n in the normalized text. Adding the number of CRLFs before a normalized offset gives the + // original offset, and a range that covers that LF covers the whole CRLF. + let crlfBefore = 0; + let nextCrlf = originalContent.indexOf("\r\n"); + const toOriginal = (offset: number): number => { + while (nextCrlf !== -1 && nextCrlf - crlfBefore < offset) { + crlfBefore++; + nextCrlf = originalContent.indexOf("\r\n", nextCrlf + 2); + } + return offset + crlfBefore; + }; + + const parts: string[] = []; + let endsWithCr = false; + const push = (part: string): void => { + if (part.length === 0) return; + // A CR before a seam and an LF after it would merge into one CRLF line break, for example a kept + // lone CR followed by inserted text that starts with LF, or both sides of a deleted range. Adding + // an LF turns the CR into a CRLF of its own, so the written file has the line breaks the diff shows. + if (endsWithCr && part[0] === "\n") parts.push("\n"); + parts.push(part); + endsWithCr = part[part.length - 1] === "\r"; + }; + let position = 0; + for (const replacement of replacements) { + const start = toOriginal(replacement.matchIndex); + push(originalContent.slice(position, start)); + const ending = getLineEndingAt(originalContent, start); + push(ending === "\n" ? replacement.newText : replacement.newText.replaceAll("\n", ending)); + position = toOriginal(replacement.matchIndex + replacement.matchLength); + } + push(originalContent.slice(position)); + return parts.join(""); +} + +/** + * Line ending of the line containing `offset`: its own LF or CRLF, or that of the line before + * for an unterminated last line. A lone CR only counts in files without any LF (classic Mac + * line endings); elsewhere it is more likely a control character inside a line. + */ +function getLineEndingAt(content: string, offset: number): "\r\n" | "\n" | "\r" { + let lineFeed = content.indexOf("\n", offset); + if (lineFeed === -1) lineFeed = content.lastIndexOf("\n", offset); + if (lineFeed === -1) return content.includes("\r") ? "\r" : "\n"; + return content[lineFeed - 1] === "\r" ? "\r\n" : "\n"; } /** @@ -70,7 +113,8 @@ interface MatchedEdit { newText: string; } -type TextReplacement = Pick; +/** Replacement of `matchLength` characters at `matchIndex` with `newText`. */ +export type TextReplacement = Pick; function getLineSpans(content: string): LineSpan[] { let offset = 0; @@ -120,21 +164,21 @@ function applyReplacements(content: string, replacements: TextReplacement[], off } /** - * Apply replacements matched against `baseContent` to `originalContent` while - * preserving unchanged line blocks from the original. + * Turn replacements matched against `baseContent` into replacements of + * `originalContent` that keep unchanged line blocks from the original. * * This is useful when `baseContent` is a normalized view of the original. Each - * replacement is widened to the lines it actually touches, those touched lines - * are rewritten from the normalized base, and all other lines are copied back - * from `originalContent`. The actual replacement ranges drive preservation so + * replacement is widened to the lines it actually touches, and those touched + * lines are rewritten from the normalized base; all other lines keep their + * original text. The actual replacement ranges drive preservation so * duplicate normalized lines cannot be aligned to the wrong occurrence. */ -export function applyReplacementsPreservingUnchangedLines( +function widenReplacementsToLines( originalContent: string, baseContent: string, replacements: TextReplacement[], -): string { - const originalLines = splitLinesWithEndings(originalContent); +): TextReplacement[] { + const originalLines = getLineSpans(originalContent); const baseLines = getLineSpans(baseContent); if (originalLines.length !== baseLines.length) { throw new Error("Cannot preserve unchanged lines because the base content has a different line count."); @@ -153,23 +197,20 @@ export function applyReplacementsPreservingUnchangedLines( groups.push({ ...range, replacements: [replacement] }); } - let originalLineIndex = 0; - let result = ""; - for (const group of groups) { - result += originalLines.slice(originalLineIndex, group.startLine).join(""); - + return groups.map((group) => { const groupStartOffset = baseLines[group.startLine].start; const groupEndOffset = baseLines[group.endLine - 1].end; - result += applyReplacements( - baseContent.slice(groupStartOffset, groupEndOffset), - group.replacements, - groupStartOffset, - ); - originalLineIndex = group.endLine; - } - result += originalLines.slice(originalLineIndex).join(""); - - return result; + const originalStart = originalLines[group.startLine].start; + return { + matchIndex: originalStart, + matchLength: originalLines[group.endLine - 1].end - originalStart, + newText: applyReplacements( + baseContent.slice(groupStartOffset, groupEndOffset), + group.replacements, + groupStartOffset, + ), + }; + }); } export interface FuzzyMatchResult { @@ -196,6 +237,8 @@ export interface Edit { export interface AppliedEditsResult { baseContent: string; newContent: string; + /** Sorted replacements of `baseContent` that turn it into `newContent` */ + replacements: TextReplacement[]; } /** @@ -350,15 +393,16 @@ export function applyEditsToNormalizedContent( } const baseContent = normalizedContent; - const newContent = usedFuzzyMatch - ? applyReplacementsPreservingUnchangedLines(normalizedContent, replacementBaseContent, matchedEdits) - : applyReplacements(replacementBaseContent, matchedEdits); + const replacements = usedFuzzyMatch + ? widenReplacementsToLines(normalizedContent, replacementBaseContent, matchedEdits) + : matchedEdits; + const newContent = applyReplacements(baseContent, replacements); if (baseContent === newContent) { throw getNoChangeError(path, normalizedEdits.length); } - return { baseContent, newContent }; + return { baseContent, newContent, replacements }; } /** Generate a standard unified patch. */ diff --git a/packages/coding-agent/src/core/tools/edit.ts b/packages/coding-agent/src/core/tools/edit.ts index bb6aeff59..3f95f76db 100644 --- a/packages/coding-agent/src/core/tools/edit.ts +++ b/packages/coding-agent/src/core/tools/edit.ts @@ -5,12 +5,11 @@ import { splitBom } from "../../utils/text.ts"; import type { ExtensionContext, ToolDefinition } from "../extensions/types.ts"; import { applyEditsToNormalizedContent, - detectLineEnding, + applyReplacementsToOriginal, type Edit, generateDiffString, generateUnifiedPatch, normalizeToLF, - restoreLineEndings, } from "./edit-diff.ts"; import { withFileMutationQueue } from "./file-mutation-queue.ts"; import { resolveToCwd } from "./path-utils.ts"; @@ -186,17 +185,24 @@ export function createEditToolDefinition( // Strip BOM before matching. The model will not include an invisible BOM in oldText. const { bom, text: content } = splitBom(rawContent); - const originalEnding = detectLineEnding(content); const normalizedContent = normalizeToLF(content); - const { baseContent, newContent } = applyEditsToNormalizedContent(normalizedContent, edits, path); + const { baseContent, newContent, replacements } = applyEditsToNormalizedContent( + normalizedContent, + edits, + path, + ); + // Diff before writing, so diff computation cannot fail after the write. + const diffResult = generateDiffString(baseContent, newContent); + const patch = generateUnifiedPatch(path, baseContent, newContent); throwIfAborted(); - const finalContent = bom + restoreLineEndings(newContent, originalEnding); + // Splice into the original text, so untouched lines keep their line endings. Without any CR + // the original is the normalized text, and the normalized result is already final. + const finalContent = + bom + (content === normalizedContent ? newContent : applyReplacementsToOriginal(content, replacements)); await ops.writeFile(absolutePath, finalContent); throwIfAborted(); - const diffResult = generateDiffString(baseContent, newContent); - const patch = generateUnifiedPatch(path, baseContent, newContent); return { content: [ { diff --git a/packages/coding-agent/test/edit-tool-diff-before-write.test.ts b/packages/coding-agent/test/edit-tool-diff-before-write.test.ts new file mode 100644 index 000000000..457dc4123 --- /dev/null +++ b/packages/coding-agent/test/edit-tool-diff-before-write.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it, vi } from "vitest"; +import { createEditTool } from "../src/core/tools/edit.ts"; + +vi.mock("../src/core/tools/edit-diff.ts", async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + generateUnifiedPatch: () => { + throw new Error("diff failed"); + }, + }; +}); + +describe("edit tool diff before write", () => { + it("should compute the diff before writing, so a diff failure leaves the file unchanged", async () => { + const writeFile = vi.fn(async () => {}); + const tool = createEditTool(process.cwd(), { + operations: { + access: async () => {}, + readFile: async () => Buffer.from("a\nb\n"), + writeFile, + }, + }); + + await expect( + tool.execute("test-diff-before-write", { path: "f.txt", edits: [{ oldText: "b", newText: "B" }] }), + ).rejects.toThrow("diff failed"); + expect(writeFile).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/coding-agent/test/tools.test.ts b/packages/coding-agent/test/tools.test.ts index 0fc908486..0c7070623 100644 --- a/packages/coding-agent/test/tools.test.ts +++ b/packages/coding-agent/test/tools.test.ts @@ -12,7 +12,12 @@ import { createLocalBashOperations, } from "../src/core/tools/bash.ts"; import { createEditToolDefinition } from "../src/core/tools/edit.ts"; -import { computeEditsDiff } from "../src/core/tools/edit-diff.ts"; +import { + applyEditsToNormalizedContent, + applyReplacementsToOriginal, + computeEditsDiff, + normalizeToLF, +} from "../src/core/tools/edit-diff.ts"; import { createFindToolDefinition, type FindOperations } from "../src/core/tools/find.ts"; import { createGrepToolDefinition } from "../src/core/tools/grep.ts"; import { createLsToolDefinition } from "../src/core/tools/ls.ts"; @@ -1714,4 +1719,141 @@ describe("edit tool CRLF handling", () => { const content = readFileSync(testFile, "utf-8"); expect(content).toBe("\uFEFFfirst\r\nSECOND\r\nthird\r\nFOURTH\r\n"); }); + + it("should write CRLF for inserted lines in CRLF files", async () => { + const testFile = join(testDir, "crlf-bytes.txt"); + writeFileSync(testFile, "first\r\nsecond\r\nthird"); + + await editTool.execute("test-crlf-bytes", { + path: testFile, + edits: [{ oldText: "second\nthird", newText: "2a\r\n2b\nthird!" }], + }); + + expect(readFileSync(testFile).equals(Buffer.from("first\r\n2a\r\n2b\r\nthird!"))).toBe(true); + }); + + it("should write LF for inserted lines in LF files", async () => { + const testFile = join(testDir, "lf-bytes.txt"); + writeFileSync(testFile, "first\nsecond\nthird"); + + await editTool.execute("test-lf-bytes", { + path: testFile, + edits: [{ oldText: "second\nthird", newText: "2a\r\n2b\nthird!" }], + }); + + expect(readFileSync(testFile).equals(Buffer.from("first\n2a\n2b\nthird!"))).toBe(true); + }); + + // #8544 + it("should keep CRLF lines when the first line ends with LF", async () => { + const testFile = join(testDir, "mixed-lf-first.txt"); + writeFileSync(testFile, "a\nb\r\nX = 1\r\n"); + + await editTool.execute("test-mixed-lf-first", { + path: testFile, + edits: [{ oldText: "X = 1", newText: "X = 2" }], + }); + + expect(readFileSync(testFile).equals(Buffer.from("a\nb\r\nX = 2\r\n"))).toBe(true); + }); + + // #8544 + it("should keep LF lines when the first line ends with CRLF", async () => { + const testFile = join(testDir, "mixed-crlf-first.txt"); + writeFileSync(testFile, "a\r\nb\nc"); + + await editTool.execute("test-mixed-crlf-first", { path: testFile, edits: [{ oldText: "c", newText: "C" }] }); + + expect(readFileSync(testFile).equals(Buffer.from("a\r\nb\nC"))).toBe(true); + }); + + // #8544 + it("should keep a lone CR in an untouched line", async () => { + const testFile = join(testDir, "lone-cr.txt"); + writeFileSync(testFile, "a\rb\nX = 1\n"); + + await editTool.execute("test-lone-cr", { path: testFile, edits: [{ oldText: "X = 1", newText: "X = 2" }] }); + + expect(readFileSync(testFile).equals(Buffer.from("a\rb\nX = 2\n"))).toBe(true); + }); + + // #8544 + it("should give inserted lines the line ending of the line they replace in multi-edit mode", async () => { + const testFile = join(testDir, "mixed-multi.txt"); + writeFileSync(testFile, "lf1\nlf2\ncr1\r\ncr2\r\n"); + + await editTool.execute("test-mixed-multi", { + path: testFile, + edits: [ + { oldText: "lf1\n", newText: "A\nB\n" }, + { oldText: "cr1\n", newText: "C\nD\n" }, + ], + }); + + expect(readFileSync(testFile).equals(Buffer.from("A\nB\nlf2\nC\r\nD\r\ncr2\r\n"))).toBe(true); + }); + + // #8544 + it("should keep untouched line endings when fuzzy matching widens the edit to whole lines", async () => { + const testFile = join(testDir, "mixed-fuzzy.txt"); + writeFileSync(testFile, "a\r\nfoo \r\nb\n"); + + await editTool.execute("test-mixed-fuzzy", { path: testFile, edits: [{ oldText: "foo\n", newText: "FOO\n" }] }); + + expect(readFileSync(testFile).equals(Buffer.from("a\r\nFOO\r\nb\n"))).toBe(true); + }); + + // A kept CR and an LF meeting at a splice seam must not merge into one CRLF line break. + it("should not merge a kept lone CR with a line feed after it", async () => { + const cases = [ + // Inserted text starting with LF after a kept lone CR. + { edit: { oldText: "b", newText: "\nB" }, written: "a\r\n\nB\nc", diffed: "a\n\nB\nc" }, + // Deleted text between a kept lone CR and a kept LF. + { edit: { oldText: "b", newText: "" }, written: "a\r\n\nc", diffed: "a\n\nc" }, + ]; + for (const [index, { edit, written, diffed }] of cases.entries()) { + const testFile = join(testDir, `cr-seam-${index}.txt`); + writeFileSync(testFile, "a\rb\nc"); + + const result = await editTool.execute("test-cr-seam", { path: testFile, edits: [edit] }); + + expect(readFileSync(testFile).equals(Buffer.from(written))).toBe(true); + expect(applyPatch("a\nb\nc", result.details?.patch ?? "")).toBe(diffed); + } + }); + + it("should not merge an inserted CR line ending with a kept line feed after it", () => { + // Only files without LF give inserted text CR line endings, so check the seam directly. + const written = applyReplacementsToOriginal("a\rb\nc", [{ matchIndex: 2, matchLength: 1, newText: "\r" }]); + + expect(normalizeToLF(written)).toBe("a\n\n\nc"); + }); + + it("should write content that normalizes to the diffed content for random mixed line endings", () => { + let seed = 1; + const random = (n: number): number => { + seed = (seed * 1103515245 + 12345) & 0x7fffffff; + // The high bits: the low bits of this generator repeat with a short period. + return Math.floor((seed / 0x80000000) * n); + }; + const endings = ["\n", "\r\n", "\r", ""]; + for (let run = 0; run < 3000; run++) { + let original = ""; + for (let line = random(12) + 1; line > 0; line--) { + original += `w${random(100)}${random(5) === 0 ? " " : ""}${endings[random(4)]}`; + } + const normalized = normalizeToLF(original); + const start = random(normalized.length); + const oldText = normalized.slice(start, start + 1 + random(12)); + const newText = ["", "\n", "X\n", "\nY", "\r", "Z"][random(6)]; + let applied: ReturnType; + try { + applied = applyEditsToNormalizedContent(normalized, [{ oldText, newText }], "f.txt"); + } catch { + continue; // duplicate, not found or no change + } + + expect(normalizeToLF(applyReplacementsToOriginal(original, applied.replacements))).toBe(applied.newContent); + } + }); });