From 53f94a78529c6ef1112b3ef4692db4a4665cf071 Mon Sep 17 00:00:00 2001 From: KJ7LNW <93454819+KJ7LNW@users.noreply.github.com> Date: Sat, 31 May 2025 13:06:41 -0700 Subject: [PATCH] fix: prevent start_line/end_line in apply_diff REPLACE (#4015) Adds validation to ensure that `:start_line:` and `:end_line:` markers do not appear in the REPLACE section of an apply_diff operation. These markers are only valid within the SEARCH section. This change prevents potential errors and confusion when users might inadvertently include these markers in the replacement content. New tests have been added to verify this validation. Fixes: #4013 Signed-off-by: Eric Wheeler Co-authored-by: Eric Wheeler --- .../__tests__/multi-search-replace.test.ts | 164 ++++++++++++++++++ .../diff/strategies/multi-search-replace.ts | 34 ++++ 2 files changed, 198 insertions(+) diff --git a/src/core/diff/strategies/__tests__/multi-search-replace.test.ts b/src/core/diff/strategies/__tests__/multi-search-replace.test.ts index dc971d3a10..7d19f37cba 100644 --- a/src/core/diff/strategies/__tests__/multi-search-replace.test.ts +++ b/src/core/diff/strategies/__tests__/multi-search-replace.test.ts @@ -2407,4 +2407,168 @@ function two() { expect(description).toContain("") }) }) + + describe("line marker validation in REPLACE sections", () => { + let strategy: MultiSearchReplaceDiffStrategy + + beforeEach(() => { + strategy = new MultiSearchReplaceDiffStrategy() + }) + + it("should reject start_line marker in REPLACE section", () => { + const diff = + "<<<<<<< SEARCH\n" + + "content to find\n" + + "=======\n" + + ":start_line:5\n" + + "replacement content\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("Invalid line marker ':start_line:' found in REPLACE section") + expect(result.error).toContain( + "Line markers (:start_line: and :end_line:) are only allowed in SEARCH sections", + ) + }) + + it("should reject end_line marker in REPLACE section", () => { + const diff = + "<<<<<<< SEARCH\n" + + "content to find\n" + + "=======\n" + + ":end_line:10\n" + + "replacement content\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("Invalid line marker ':end_line:' found in REPLACE section") + expect(result.error).toContain( + "Line markers (:start_line: and :end_line:) are only allowed in SEARCH sections", + ) + }) + + it("should reject both line markers in REPLACE section", () => { + const diff = + "<<<<<<< SEARCH\n" + + "content to find\n" + + "=======\n" + + ":start_line:5\n" + + ":end_line:10\n" + + "replacement content\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("Invalid line marker ':start_line:' found in REPLACE section") + }) + + it("should reject line markers in multiple diff blocks where one has invalid markers", () => { + const diff = + "<<<<<<< SEARCH\n" + + ":start_line:1\n" + + "content1\n" + + "=======\n" + + "replacement1\n" + + ">>>>>>> REPLACE\n\n" + + "<<<<<<< SEARCH\n" + + "content2\n" + + "=======\n" + + ":start_line:5\n" + + "replacement2\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("Invalid line marker ':start_line:' found in REPLACE section") + }) + + it("should allow valid markers in SEARCH section with content in REPLACE", () => { + const diff = + "<<<<<<< SEARCH\n" + + ":start_line:5\n" + + ":end_line:10\n" + + "-------\n" + + "content to find\n" + + "=======\n" + + "replacement content\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(true) + }) + + it("should allow escaped line markers in REPLACE content", () => { + const diff = + "<<<<<<< SEARCH\n" + + "content to find\n" + + "=======\n" + + "replacement content\n" + + "\\:start_line:5\n" + + "more content\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(true) + }) + + it("should allow escaped end_line markers in REPLACE content", () => { + const diff = + "<<<<<<< SEARCH\n" + + "content to find\n" + + "=======\n" + + "replacement content\n" + + "\\:end_line:10\n" + + "more content\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(true) + }) + + it("should allow both escaped line markers in REPLACE content", () => { + const diff = + "<<<<<<< SEARCH\n" + + "content to find\n" + + "=======\n" + + "replacement content\n" + + "\\:start_line:5\n" + + "\\:end_line:10\n" + + "more content\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(true) + }) + + it("should reject line markers with whitespace in REPLACE section", () => { + const diff = + "<<<<<<< SEARCH\n" + + "content to find\n" + + "=======\n" + + " :start_line:5 \n" + + "replacement content\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("Invalid line marker ':start_line:' found in REPLACE section") + }) + + it("should reject line markers in middle of REPLACE content", () => { + const diff = + "<<<<<<< SEARCH\n" + + "content to find\n" + + "=======\n" + + "some replacement\n" + + ":end_line:15\n" + + "more replacement\n" + + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("Invalid line marker ':end_line:' found in REPLACE section") + }) + + it("should provide helpful error message format", () => { + const diff = + "<<<<<<< SEARCH\n" + "content\n" + "=======\n" + ":start_line:5\n" + "replacement\n" + ">>>>>>> REPLACE" + const result = strategy["validateMarkerSequencing"](diff) + expect(result.success).toBe(false) + expect(result.error).toContain("CORRECT FORMAT:") + expect(result.error).toContain("INCORRECT FORMAT:") + expect(result.error).toContain(":start_line:5 <-- Invalid location") + }) + }) }) diff --git a/src/core/diff/strategies/multi-search-replace.ts b/src/core/diff/strategies/multi-search-replace.ts index af5a03e468..9e740a6571 100644 --- a/src/core/diff/strategies/multi-search-replace.ts +++ b/src/core/diff/strategies/multi-search-replace.ts @@ -243,6 +243,30 @@ Only use a single line of '=======' between search and replacement content, beca ">>>>>>> REPLACE\n", }) + const reportLineMarkerInReplaceError = (marker: string) => ({ + success: false, + error: + `ERROR: Invalid line marker '${marker}' found in REPLACE section at line ${state.line}\n` + + "\n" + + "Line markers (:start_line: and :end_line:) are only allowed in SEARCH sections.\n" + + "\n" + + "CORRECT FORMAT:\n" + + "<<<<<<< SEARCH\n" + + ":start_line:5\n" + + "content to find\n" + + "=======\n" + + "replacement content\n" + + ">>>>>>> REPLACE\n" + + "\n" + + "INCORRECT FORMAT:\n" + + "<<<<<<< SEARCH\n" + + "content to find\n" + + "=======\n" + + ":start_line:5 <-- Invalid location\n" + + "replacement content\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 @@ -254,6 +278,16 @@ Only use a single line of '=======' between search and replacement content, beca state.line++ const marker = line.trim() + // Check for line markers in REPLACE sections (but allow escaped ones) + if (state.current === State.AFTER_SEPARATOR) { + if (marker.startsWith(":start_line:") && !line.trim().startsWith("\\:start_line:")) { + return reportLineMarkerInReplaceError(":start_line:") + } + if (marker.startsWith(":end_line:") && !line.trim().startsWith("\\:end_line:")) { + return reportLineMarkerInReplaceError(":end_line:") + } + } + switch (state.current) { case State.START: if (marker === SEP)