From f2a6e4295c60ac37795291df4b1ca1e5829b56c8 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sun, 7 Sep 2025 04:06:09 +0000 Subject: [PATCH] fix: improve error messages for diff marker escaping in apply_diff tool - Enhanced error messages to clearly explain when and how to escape special markers - Differentiated between structural errors and content escaping issues - Added helpful examples showing correct escaping syntax - Added comprehensive tests for marker escaping scenarios - Fixes issue where Grok Coder models get confused by unescaped markers in content Resolves #7750 --- .../__tests__/escaping-markers.spec.ts | 422 ++++++++++++++++++ .../__tests__/multi-search-replace.spec.ts | 18 +- .../strategies/multi-file-search-replace.ts | 56 ++- .../diff/strategies/multi-search-replace.ts | 56 ++- 4 files changed, 506 insertions(+), 46 deletions(-) create mode 100644 src/core/diff/strategies/__tests__/escaping-markers.spec.ts diff --git a/src/core/diff/strategies/__tests__/escaping-markers.spec.ts b/src/core/diff/strategies/__tests__/escaping-markers.spec.ts new file mode 100644 index 0000000000..20760ec9af --- /dev/null +++ b/src/core/diff/strategies/__tests__/escaping-markers.spec.ts @@ -0,0 +1,422 @@ +import { MultiSearchReplaceDiffStrategy } from "../multi-search-replace" +import { MultiFileSearchReplaceDiffStrategy } from "../multi-file-search-replace" + +describe("Escaping diff markers in content", () => { + describe("MultiSearchReplaceDiffStrategy", () => { + let strategy: MultiSearchReplaceDiffStrategy + + beforeEach(() => { + strategy = new MultiSearchReplaceDiffStrategy() + }) + + it("should handle escaped equals separator in search content", async () => { + const originalContent = `function test() { +======= + return true; +}` + // Create diff content with escaped marker + const searchBlock = + "<<<<<<< SEARCH\n" + + "function test() {\n" + + "\\=======\n" + + " return true;\n" + + "}\n" + + "=======\n" + + "function test() {\n" + + " // Updated comment\n" + + " return false;\n" + + "}\n" + + ">>>>>>> REPLACE" + + const result = await strategy.applyDiff(originalContent, searchBlock) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function test() { + // Updated comment + return false; +}`) + } + }) + + it("should handle escaped search marker in content", async () => { + const originalContent = `// Example of diff markers +<<<<<<< SEARCH +// This is content` + + const searchBlock = + "<<<<<<< SEARCH\n" + + "// Example of diff markers\n" + + "\\<<<<<<< SEARCH\n" + + "// This is content\n" + + "=======\n" + + "// Example of diff markers\n" + + "// [DIFF MARKER REMOVED]\n" + + "// This is content\n" + + ">>>>>>> REPLACE" + + const result = await strategy.applyDiff(originalContent, searchBlock) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`// Example of diff markers +// [DIFF MARKER REMOVED] +// This is content`) + } + }) + + it("should handle escaped replace marker in content", async () => { + const originalContent = `function merge() { +>>>>>>> REPLACE + return "merged"; +}` + + const searchBlock = + "<<<<<<< SEARCH\n" + + "function merge() {\n" + + "\\>>>>>>> REPLACE\n" + + ' return "merged";\n' + + "}\n" + + "=======\n" + + "function merge() {\n" + + " // Conflict resolved\n" + + ' return "resolved";\n' + + "}\n" + + ">>>>>>> REPLACE" + + const result = await strategy.applyDiff(originalContent, searchBlock) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function merge() { + // Conflict resolved + return "resolved"; +}`) + } + }) + + it("should handle multiple escaped markers in the same content", async () => { + const originalContent = `// Git merge conflict example +<<<<<<< HEAD +content from HEAD +======= +content from branch +>>>>>>> feature-branch` + + const searchBlock = + "<<<<<<< SEARCH\n" + + "// Git merge conflict example\n" + + "\\<<<<<<< HEAD\n" + + "content from HEAD\n" + + "\\=======\n" + + "content from branch\n" + + "\\>>>>>>> feature-branch\n" + + "=======\n" + + "// Git merge conflict resolved\n" + + "// content merged successfully\n" + + ">>>>>>> REPLACE" + + const result = await strategy.applyDiff(originalContent, searchBlock) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`// Git merge conflict resolved +// content merged successfully`) + } + }) + + it("should handle escaped dashes separator in content", async () => { + const originalContent = `function divider() { +------- + return "divided"; +}` + + const searchBlock = + "<<<<<<< SEARCH\n" + + "function divider() {\n" + + "\\-------\n" + + ' return "divided";\n' + + "}\n" + + "=======\n" + + "function divider() {\n" + + " // Separator line\n" + + ' return "divided";\n' + + "}\n" + + ">>>>>>> REPLACE" + + const result = await strategy.applyDiff(originalContent, searchBlock) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function divider() { + // Separator line + return "divided"; +}`) + } + }) + + it("should fail when equals marker is not escaped in search content", () => { + const diff = + "<<<<<<< SEARCH\n" + + "function test() {\n" + + "=======\n" + + " return true;\n" + + "}\n" + + "=======\n" + + "function test() {\n" + + " return false;\n" + + "}\n" + + ">>>>>>> REPLACE" + + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("The marker '=======' at line") + expect(result.error).toContain("appears to be part of the content you're trying to edit") + expect(result.error).toContain("you MUST escape it by adding a backslash") + }) + + it("should fail when search marker is not escaped in content", () => { + const diff = + "<<<<<<< SEARCH\n" + + "// Example\n" + + "<<<<<<< HEAD\n" + + "// content\n" + + "=======\n" + + "// Updated\n" + + ">>>>>>> REPLACE" + + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("appears to be part of the content you're trying to edit") + expect(result.error).toContain("\\<<<<<<< SEARCH") + }) + + it("should provide helpful error message for unescaped markers", () => { + // This is actually a valid diff structure, not an error case + const diff = "<<<<<<< SEARCH\n" + "code with\n" + "=======\n" + "replacement\n" + ">>>>>>> REPLACE" + + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(true) // This is valid + }) + + it("should handle escaped markers in replace content", async () => { + const originalContent = `function test() { + return true; +}` + + const searchBlock = + "<<<<<<< SEARCH\n" + + "function test() {\n" + + " return true;\n" + + "}\n" + + "=======\n" + + "function test() {\n" + + "\\=======\n" + + "\\<<<<<<< SEARCH\n" + + "\\>>>>>>> REPLACE\n" + + " return false;\n" + + "}\n" + + ">>>>>>> REPLACE" + + const result = await strategy.applyDiff(originalContent, searchBlock) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function test() { +======= +<<<<<<< SEARCH +>>>>>>> REPLACE + return false; +}`) + } + }) + + it("should handle real-world merge conflict removal scenario", async () => { + const originalContent = `function calculate(a, b) { +<<<<<<< HEAD + return a + b; +======= + return a * b; +>>>>>>> feature-branch +}` + + const searchBlock = + "<<<<<<< SEARCH\n" + + "function calculate(a, b) {\n" + + "\\<<<<<<< HEAD\n" + + " return a + b;\n" + + "\\=======\n" + + " return a * b;\n" + + "\\>>>>>>> feature-branch\n" + + "}\n" + + "=======\n" + + "function calculate(a, b) {\n" + + " // Resolved: using multiplication\n" + + " return a * b;\n" + + "}\n" + + ">>>>>>> REPLACE" + + const result = await strategy.applyDiff(originalContent, searchBlock) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function calculate(a, b) { + // Resolved: using multiplication + return a * b; +}`) + } + }) + }) + + describe("MultiFileSearchReplaceDiffStrategy", () => { + let strategy: MultiFileSearchReplaceDiffStrategy + + beforeEach(() => { + strategy = new MultiFileSearchReplaceDiffStrategy() + }) + + it("should handle escaped markers in multi-file strategy", async () => { + const originalContent = `function test() { +======= + return true; +}` + + const searchBlock = + "<<<<<<< SEARCH\n" + + "function test() {\n" + + "\\=======\n" + + " return true;\n" + + "}\n" + + "=======\n" + + "function test() {\n" + + " // Updated\n" + + " return false;\n" + + "}\n" + + ">>>>>>> REPLACE" + + const result = await strategy["applySingleDiff"](originalContent, searchBlock) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function test() { + // Updated + return false; +}`) + } + }) + + it("should validate and reject unescaped markers", () => { + const diff = + "<<<<<<< SEARCH\n" + + "content with\n" + + "=======\n" + + "inside\n" + + "=======\n" + + "replacement\n" + + ">>>>>>> REPLACE" + + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("The marker '=======' at line") + expect(result.error).toContain("appears to be part of the content you're trying to edit") + }) + + it("should handle array of diffs with escaped markers", async () => { + const originalContent = `function one() { +======= + return 1; +} + +function two() { +<<<<<<< SEARCH + return 2; +}` + + const diffs = [ + { + content: + "<<<<<<< SEARCH\n" + + "function one() {\n" + + "\\=======\n" + + " return 1;\n" + + "}\n" + + "=======\n" + + "function one() {\n" + + " // Separator removed\n" + + " return 1;\n" + + "}\n" + + ">>>>>>> REPLACE", + startLine: 1, + }, + { + content: + "<<<<<<< SEARCH\n" + + "function two() {\n" + + "\\<<<<<<< SEARCH\n" + + " return 2;\n" + + "}\n" + + "=======\n" + + "function two() {\n" + + " // Marker removed\n" + + " return 2;\n" + + "}\n" + + ">>>>>>> REPLACE", + startLine: 6, + }, + ] + + const result = await strategy.applyDiff(originalContent, diffs) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function one() { + // Separator removed + return 1; +} + +function two() { + // Marker removed + return 2; +}`) + } + }) + }) + + describe("Error message improvements", () => { + let strategy: MultiSearchReplaceDiffStrategy + + beforeEach(() => { + strategy = new MultiSearchReplaceDiffStrategy() + }) + + it("should provide clear guidance when encountering unescaped markers", () => { + // Test with actual unescaped marker in content + const diff = + "<<<<<<< SEARCH\n" + + "// This code has\n" + + "=======\n" + + "// More content\n" + + "=======\n" + // This extra ======= should trigger an error + "// Fixed\n" + + ">>>>>>> REPLACE" + + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + + // Check for improved error message components + expect(result.error).toContain("appears to be part of the content") + expect(result.error).toContain("MUST escape it") + }) + + it("should distinguish between structural errors and content escaping issues", () => { + // Structural error (missing SEARCH marker) + const structuralError = "=======\n" + "content\n" + ">>>>>>> REPLACE" + + const structResult = strategy["validateMarkerSequencing"](structuralError) + expect(structResult.success).toBe(false) + expect(structResult.error).toContain("Diff structure is incorrect") + expect(structResult.error).toContain("CORRECT DIFF STRUCTURE") + expect(structResult.error).toContain("COMMON ISSUES") + + // Content escaping issue - markers on their own lines + const escapingError = + "<<<<<<< SEARCH\n" + "content\n" + "=======\n" + "more content\n" + "=======\n" + ">>>>>>> REPLACE" + + const escapeResult = strategy["validateMarkerSequencing"](escapingError) + expect(escapeResult.success).toBe(false) + expect(escapeResult.error).toContain("appears to be part of the content") + expect(escapeResult.error).toContain("MUST escape it") + }) + }) +}) diff --git a/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts b/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts index b25286f5fa..21f546d17b 100644 --- a/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts +++ b/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts @@ -96,32 +96,34 @@ describe("MultiSearchReplaceDiffStrategy", () => { const diff = "=======\n" + "content\n" + ">>>>>>> REPLACE" const result = strategy["validateMarkerSequencing"](diff) expect(result.success).toBe(false) - expect(result.error).toContain("'=======' found in your diff content") - expect(result.error).toContain("Diff block is malformed") + expect(result.error).toContain("Diff structure is incorrect") + expect(result.error).toContain("Found: '======='") + expect(result.error).toContain("Expected: '<<<<<<< SEARCH") }) it("detects missing separator", () => { const diff = "<<<<<<< SEARCH\n" + "content\n" + ">>>>>>> REPLACE" const result = strategy["validateMarkerSequencing"](diff) expect(result.success).toBe(false) - expect(result.error).toContain("'>>>>>>> REPLACE' found in your diff content") - expect(result.error).toContain("Diff block is malformed") + expect(result.error).toContain("Diff structure is incorrect") + expect(result.error).toContain("Found: '>>>>>>> REPLACE'") + expect(result.error).toContain("Expected: '======='") }) it("detects two separators", () => { const diff = "<<<<<<< SEARCH\n" + "content\n" + "=======\n" + "=======\n" + ">>>>>>> REPLACE" const result = strategy["validateMarkerSequencing"](diff) expect(result.success).toBe(false) - expect(result.error).toContain("'=======' found in your diff content") - expect(result.error).toContain("When removing merge conflict markers") + expect(result.error).toContain("appears to be part of the content") + expect(result.error).toContain("MUST escape it") }) it("detects replace before separator (merge conflict message)", () => { const diff = "<<<<<<< SEARCH\n" + "content\n" + ">>>>>>>" const result = strategy["validateMarkerSequencing"](diff) expect(result.success).toBe(false) - expect(result.error).toContain("'>>>>>>>' found in your diff content") - expect(result.error).toContain("When removing merge conflict markers") + expect(result.error).toContain("appears to be part of the content") + expect(result.error).toContain("MUST escape it") }) it("detects incomplete sequence", () => { diff --git a/src/core/diff/strategies/multi-file-search-replace.ts b/src/core/diff/strategies/multi-file-search-replace.ts index a212cf2b8e..ba75a05203 100644 --- a/src/core/diff/strategies/multi-file-search-replace.ts +++ b/src/core/diff/strategies/multi-file-search-replace.ts @@ -263,40 +263,58 @@ Each file requires its own path, start_line, and diff elements. const reportMergeConflictError = (found: string, _expected: string) => ({ success: false, error: - `ERROR: Special marker '${found}' found in your diff content at line ${state.line}:\n` + + `ERROR: The marker '${found}' at line ${state.line} appears to be part of the content you're trying to edit.\n` + "\n" + - `When removing merge conflict markers like '${found}' from files, you MUST escape them\n` + - "in your SEARCH section by prepending a backslash (\\) at the beginning of the line:\n" + + `If '${found}' is part of the actual file content (not a diff marker), you MUST escape it by adding a backslash (\\) at the beginning of the line.\n` + "\n" + - "CORRECT FORMAT:\n\n" + + "EXAMPLE - If your file contains '=======' as actual content:\n\n" + "<<<<<<< SEARCH\n" + - "content before\n" + - `\\${found} <-- Note the backslash here in this example\n` + - "content after\n" + + "function example() {\n" + + ` // ${found}\n` + + ` \\${found} <-- Add backslash to escape\n` + + " return true;\n" + + "}\n" + "=======\n" + - "replacement content\n" + + "function example() {\n" + + " // Updated comment\n" + + " return false;\n" + + "}\n" + ">>>>>>> REPLACE\n" + "\n" + - "Without escaping, the system confuses your content with diff syntax markers.\n" + - "You may use multiple diff blocks in a single diff request, but ANY of ONLY the following separators that occur within SEARCH or REPLACE content must be escaped, as follows:\n" + - `\\${SEARCH}\n` + - `\\${SEP}\n` + - `\\${REPLACE}\n`, + "The following markers MUST be escaped when they appear as content (not as diff syntax):\n" + + `• \\<<<<<<< SEARCH (or any line starting with <<<<<<<)\n` + + `• \\======= (exactly seven equals signs)\n` + + `• \\>>>>>>> REPLACE (or any line starting with >>>>>>>)\n` + + `• \\------- (exactly seven dashes)\n` + + "\n" + + "TIP: Use read_file first to see the exact content, then escape any of these markers that appear in the actual file.", }) const reportInvalidDiffError = (found: string, expected: string) => ({ success: false, error: - `ERROR: Diff block is malformed: marker '${found}' found in your diff content at line ${state.line}. Expected: ${expected}\n` + + `ERROR: Diff structure is incorrect at line ${state.line}.\n` + + `Found: '${found}'\n` + + `Expected: '${expected}'\n` + "\n" + - "CORRECT FORMAT:\n\n" + + "CORRECT DIFF STRUCTURE:\n" + + "```\n" + "<<<<<<< SEARCH\n" + - ":start_line: (required) The line number of original content where the search block starts.\n" + - "-------\n" + - "[exact content to find including whitespace]\n" + + ":start_line:NUMBER (optional but recommended)\n" + + "------- (optional separator)\n" + + "[exact content to find]\n" + "=======\n" + "[new content to replace with]\n" + - ">>>>>>> REPLACE\n", + ">>>>>>> REPLACE\n" + + "```\n" + + "\n" + + "COMMON ISSUES:\n" + + `• If '${found}' is part of the file content (not a marker), escape it with a backslash: \\${found}\n` + + `• Ensure markers are on their own lines\n` + + `• Don't add extra '=' signs (must be exactly 7)\n` + + `• Check that you have matching SEARCH and REPLACE blocks\n` + + "\n" + + "TIP: If you're seeing this error repeatedly, the content you're trying to edit likely contains these special markers. Use read_file to check, then escape them.", }) const reportLineMarkerInReplaceError = (marker: string) => ({ diff --git a/src/core/diff/strategies/multi-search-replace.ts b/src/core/diff/strategies/multi-search-replace.ts index a6a9913203..376fdbc1dd 100644 --- a/src/core/diff/strategies/multi-search-replace.ts +++ b/src/core/diff/strategies/multi-search-replace.ts @@ -210,40 +210,58 @@ Only use a single line of '=======' between search and replacement content, beca const reportMergeConflictError = (found: string, _expected: string) => ({ success: false, error: - `ERROR: Special marker '${found}' found in your diff content at line ${state.line}:\n` + + `ERROR: The marker '${found}' at line ${state.line} appears to be part of the content you're trying to edit.\n` + "\n" + - `When removing merge conflict markers like '${found}' from files, you MUST escape them\n` + - "in your SEARCH section by prepending a backslash (\\) at the beginning of the line:\n" + + `If '${found}' is part of the actual file content (not a diff marker), you MUST escape it by adding a backslash (\\) at the beginning of the line.\n` + "\n" + - "CORRECT FORMAT:\n\n" + + "EXAMPLE - If your file contains '=======' as actual content:\n\n" + "<<<<<<< SEARCH\n" + - "content before\n" + - `\\${found} <-- Note the backslash here in this example\n` + - "content after\n" + + "function example() {\n" + + ` // ${found}\n` + + ` \\${found} <-- Add backslash to escape\n` + + " return true;\n" + + "}\n" + "=======\n" + - "replacement content\n" + + "function example() {\n" + + " // Updated comment\n" + + " return false;\n" + + "}\n" + ">>>>>>> REPLACE\n" + "\n" + - "Without escaping, the system confuses your content with diff syntax markers.\n" + - "You may use multiple diff blocks in a single diff request, but ANY of ONLY the following separators that occur within SEARCH or REPLACE content must be escaped, as follows:\n" + - `\\${SEARCH}\n` + - `\\${SEP}\n` + - `\\${REPLACE}\n`, + "The following markers MUST be escaped when they appear as content (not as diff syntax):\n" + + `• \\<<<<<<< SEARCH (or any line starting with <<<<<<<)\n` + + `• \\======= (exactly seven equals signs)\n` + + `• \\>>>>>>> REPLACE (or any line starting with >>>>>>>)\n` + + `• \\------- (exactly seven dashes)\n` + + "\n" + + "TIP: Use read_file first to see the exact content, then escape any of these markers that appear in the actual file.", }) const reportInvalidDiffError = (found: string, expected: string) => ({ success: false, error: - `ERROR: Diff block is malformed: marker '${found}' found in your diff content at line ${state.line}. Expected: ${expected}\n` + + `ERROR: Diff structure is incorrect at line ${state.line}.\n` + + `Found: '${found}'\n` + + `Expected: '${expected}'\n` + "\n" + - "CORRECT FORMAT:\n\n" + + "CORRECT DIFF STRUCTURE:\n" + + "```\n" + "<<<<<<< SEARCH\n" + - ":start_line: (required) The line number of original content where the search block starts.\n" + - "-------\n" + - "[exact content to find including whitespace]\n" + + ":start_line:NUMBER (optional but recommended)\n" + + "------- (optional separator)\n" + + "[exact content to find]\n" + "=======\n" + "[new content to replace with]\n" + - ">>>>>>> REPLACE\n", + ">>>>>>> REPLACE\n" + + "```\n" + + "\n" + + "COMMON ISSUES:\n" + + `• If '${found}' is part of the file content (not a marker), escape it with a backslash: \\${found}\n` + + `• Ensure markers are on their own lines\n` + + `• Don't add extra '=' signs (must be exactly 7)\n` + + `• Check that you have matching SEARCH and REPLACE blocks\n` + + "\n" + + "TIP: If you're seeing this error repeatedly, the content you're trying to edit likely contains these special markers. Use read_file to check, then escape them.", }) const reportLineMarkerInReplaceError = (marker: string) => ({