fix: Improve error message when apply_diff structure is invalid (#2260)

Currently all errors in apply_diff diff structure are assumed to be
related to merge conflicts. This is unfortunate because it is way more
likely that LLM simply made an error structuring the diff. The LLM is
then directed to fix escaping in the diff from which it doesn't recover.

It is safe to assume that missing or extra "<<<<<<< SEARCH" or ">>>>>>>
REPLACE" strings are likely bad diff structure, because while "<<<<<<<"
and ">>>>>>>" are common merge conflict markers, no tools add "SEARCH"
and "REPLACE" strings after them. This is likely output of the LLM
itself.

LLM not recovering has been observed in Claude 3.5 Sonnet and Claude 3.7
Sonnet models.

To address the issue, error message now mentions merge conflict markers
and their escaping only if it's clear that errors may come from this
source. In the rest of cases the error message repeats the expected diff
structure back to the LLM.

Author: p12tic

src/core/diff/strategies/__tests__/multi-search-replace.test.ts

@@ -28,18 +28,57 @@ describe("MultiSearchReplaceDiffStrategy", () => {
 			expect(strategy["validateMarkerSequencing"](diff).success).toBe(true)
 		})
 
+		it("validates multiple correct marker sequences with line numbers", () => {
+			const diff =
+				"<<<<<<< SEARCH\n" +
+				":start_line:10\n" +
+				":end_line:11\n" +
+				"-------\n" +
+				"content1\n" +
+				"=======\n" +
+				"new1\n" +
+				">>>>>>> REPLACE\n\n" +
+				"<<<<<<< SEARCH\n" +
+				":start_line:10\n" +
+				":end_line:11\n" +
+				"-------\n" +
+				"content2\n" +
+				"=======\n" +
+				"new2\n" +
+				">>>>>>> REPLACE"
+			expect(strategy["validateMarkerSequencing"](diff).success).toBe(true)
+		})
+
 		it("detects separator before search", () => {
 			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")
 		})
 
-		it("detects replace before separator", () => {
+		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")
+		})
+
+		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")
+		})
+
+		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")
 		})
 
 		it("detects incomplete sequence", () => {

src/core/diff/strategies/multi-search-replace.ts

@@ -162,8 +162,10 @@ Only use a single line of '=======' between search and replacement content, beca
 		const SEARCH = "<<<<<<< SEARCH"
 		const SEP = "======="
 		const REPLACE = ">>>>>>> REPLACE"
+		const SEARCH_PREFIX = "<<<<<<<"
+		const REPLACE_PREFIX = ">>>>>>>"
 
-		const reportError = (found: string, expected: string) => ({
+		const reportMergeConflictError = (found: string, expected: string) => ({
 			success: false,
 			error:
 				`ERROR: Special marker '${found}' found in your diff content at line ${state.line}:\n` +
@@ -187,27 +189,62 @@ Only use a single line of '=======' between search and replacement content, beca
 				`\\${REPLACE}\n`,
 		})
 
+		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` +
+				"\n" +
+				"CORRECT FORMAT:\n\n" +
+				"<<<<<<< SEARCH\n" +
+				":start_line: (required) The line number of original content where the search block starts.\n" +
+				":end_line: (required) The line number of original content  where the search block ends.\n" +
+				"-------\n" +
+				"[exact content to find including whitespace]\n" +
+				"=======\n" +
+				"[new content to replace with]\n" +
+				">>>>>>> REPLACE\n",
+		})
+
+		const lines = diffContent.split("\n")
+		const searchCount = lines.filter((l) => l.trim() === SEARCH).length
+		const sepCount = lines.filter((l) => l.trim() === SEP).length
+		const replaceCount = lines.filter((l) => l.trim() === REPLACE).length
+
+		const likelyBadStructure = searchCount !== replaceCount || sepCount < searchCount
+
 		for (const line of diffContent.split("\n")) {
 			state.line++
 			const marker = line.trim()
 
 			switch (state.current) {
 				case State.START:
-					if (marker === SEP) return reportError(SEP, SEARCH)
-					if (marker === REPLACE) return reportError(REPLACE, SEARCH)
+					if (marker === SEP)
+						return likelyBadStructure
+							? reportInvalidDiffError(SEP, SEARCH)
+							: reportMergeConflictError(SEP, SEARCH)
+					if (marker === REPLACE) return reportInvalidDiffError(REPLACE, SEARCH)
+					if (marker.startsWith(REPLACE_PREFIX)) return reportMergeConflictError(marker, SEARCH)
 					if (marker === SEARCH) state.current = State.AFTER_SEARCH
+					else if (marker.startsWith(SEARCH_PREFIX)) return reportMergeConflictError(marker, SEARCH)
 					break
 
 				case State.AFTER_SEARCH:
-					if (marker === SEARCH) return reportError(SEARCH, SEP)
-					if (marker === REPLACE) return reportError(REPLACE, SEP)
+					if (marker === SEARCH) return reportInvalidDiffError(SEARCH, SEP)
+					if (marker.startsWith(SEARCH_PREFIX)) return reportMergeConflictError(marker, SEARCH)
+					if (marker === REPLACE) return reportInvalidDiffError(REPLACE, SEP)
+					if (marker.startsWith(REPLACE_PREFIX)) return reportMergeConflictError(marker, SEARCH)
 					if (marker === SEP) state.current = State.AFTER_SEPARATOR
 					break
 
 				case State.AFTER_SEPARATOR:
-					if (marker === SEARCH) return reportError(SEARCH, REPLACE)
-					if (marker === SEP) return reportError(SEP, REPLACE)
+					if (marker === SEARCH) return reportInvalidDiffError(SEARCH, REPLACE)
+					if (marker.startsWith(SEARCH_PREFIX)) return reportMergeConflictError(marker, REPLACE)
+					if (marker === SEP)
+						return likelyBadStructure
+							? reportInvalidDiffError(SEP, REPLACE)
+							: reportMergeConflictError(SEP, REPLACE)
 					if (marker === REPLACE) state.current = State.START
+					else if (marker.startsWith(REPLACE_PREFIX)) return reportMergeConflictError(marker, REPLACE)
 					break
 			}
 		}