mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-24 00:51:34 +00:00
fix(apply_diff): detect malformed ------- separator and surface helpful error
Closes #12210. When a smaller model emits an apply_diff payload where the `-------` separator is missing the trailing newline, the outer regex's optional separator group quietly absorbs the run-on line into the search content. Match then fails with a confusing "63% similar (needs 100%)" error and the model loops. Add `detectMalformedSeparator` that runs against each replacement's search content right after marker unescaping. It flags first lines that start with seven or more dashes followed by non-dash, non-whitespace content on the same line. Markdown HRs and bare separators are NOT flagged. When detected, return an actionable error naming the exact problem and showing the correct shape so the model can self-correct instead of looping. Three regression tests under `malformed \`-------\` separator detection`: - exact reproduction from the issue → "Malformed separator" error surfaced; "63% similar" message no longer appears - well-formed separator still applies cleanly - search content with a long row of dashes inside the file is NOT misclassified
This commit is contained in:
parent
ad25634905
commit
6fff6d9483
2 changed files with 140 additions and 0 deletions
|
|
@ -1204,4 +1204,80 @@ function sum(a, b) {
|
|||
expect(result.error).toContain(":start_line:5 <-- Invalid location")
|
||||
})
|
||||
})
|
||||
|
||||
// Regression for https://github.com/RooCodeInc/Roo-Code/issues/12210.
|
||||
describe("malformed `-------` separator detection", () => {
|
||||
let strategy: MultiSearchReplaceDiffStrategy
|
||||
|
||||
beforeEach(() => {
|
||||
strategy = new MultiSearchReplaceDiffStrategy(1.0, 5)
|
||||
})
|
||||
|
||||
it("returns a 'malformed separator' error when LLM omits the newline after -------", async () => {
|
||||
const originalContent =
|
||||
"import { useTranslate } from '../../i18n/I18nContext';\n" +
|
||||
"\n" +
|
||||
"type MouseMode = 'draw' | 'erase';\n"
|
||||
const diffContent =
|
||||
"<<<<<<< SEARCH\n" +
|
||||
":start_line:1\n" +
|
||||
"-------import { useTranslate } from '../../i18n/I18nContext';\n" +
|
||||
"\n" +
|
||||
"type MouseMode\n" +
|
||||
"=======\n" +
|
||||
"import { useTranslate } from '../../i18n/I18nContext';\n" +
|
||||
"import { MaskEditorProvider, useMaskEditor } from './MaskEditorContext';\n" +
|
||||
"\n" +
|
||||
"type MouseMode\n" +
|
||||
">>>>>>> REPLACE"
|
||||
|
||||
const result = await strategy.applyDiff(originalContent, diffContent)
|
||||
expect(result.success).toBe(false)
|
||||
if (!result.success) {
|
||||
const parts = result.failParts ?? []
|
||||
const errors = parts
|
||||
.filter((part) => part.success === false)
|
||||
.map((part) => ("error" in part ? (part.error ?? "") : ""))
|
||||
.concat("error" in result ? (result.error ?? "") : "")
|
||||
.join(" ")
|
||||
expect(errors).toContain("Malformed separator")
|
||||
expect(errors).toContain("must be on its own line")
|
||||
expect(errors).not.toContain("63%")
|
||||
expect(errors).not.toContain("similar")
|
||||
}
|
||||
})
|
||||
|
||||
it("does not flag a well-formed -------\\n separator", async () => {
|
||||
const originalContent = "function hello() {\n console.log('hello')\n}\n"
|
||||
const diffContent =
|
||||
"<<<<<<< SEARCH\n" +
|
||||
":start_line:1\n" +
|
||||
"-------\n" +
|
||||
"function hello() {\n" +
|
||||
"=======\n" +
|
||||
"function helloWorld() {\n" +
|
||||
">>>>>>> REPLACE"
|
||||
|
||||
const result = await strategy.applyDiff(originalContent, diffContent)
|
||||
expect(result.success).toBe(true)
|
||||
if (result.success) {
|
||||
expect(result.content).toBe("function helloWorld() {\n console.log('hello')\n}\n")
|
||||
}
|
||||
})
|
||||
|
||||
it("does not flag a search line that legitimately contains many dashes", async () => {
|
||||
const originalContent = "/* ---------- header ---------- */\nconst x = 1;\n"
|
||||
const diffContent =
|
||||
"<<<<<<< SEARCH\n" +
|
||||
":start_line:1\n" +
|
||||
"-------\n" +
|
||||
"/* ---------- header ---------- */\n" +
|
||||
"=======\n" +
|
||||
"/* === header === */\n" +
|
||||
">>>>>>> REPLACE"
|
||||
|
||||
const result = await strategy.applyDiff(originalContent, diffContent)
|
||||
expect(result.success).toBe(true)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -8,6 +8,45 @@ import { normalizeString } from "../../../utils/text-normalization"
|
|||
|
||||
const BUFFER_LINES = 40 // Number of extra context lines to show before and after matches
|
||||
|
||||
/**
|
||||
* Detect a malformed `-------` separator in the SEARCH section content.
|
||||
*
|
||||
* If the LLM forgets the newline after the separator, the run-on line
|
||||
* (e.g. `-------import { ... }`) ends up as the first line of the
|
||||
* captured search content. The outer regex doesn't reject this — it
|
||||
* just falls through, and the file-content match fails with a
|
||||
* confusing "63% similar" error. By detecting the malformed prefix
|
||||
* here we can return an actionable error instead.
|
||||
*
|
||||
* Returns the offending line when malformed, or `null` otherwise.
|
||||
*
|
||||
* Regression test for https://github.com/RooCodeInc/Roo-Code/issues/12210.
|
||||
*/
|
||||
function detectMalformedSeparator(searchContent: string): string | null {
|
||||
if (!searchContent) {
|
||||
return null
|
||||
}
|
||||
const firstLine = searchContent.split(/\r?\n/, 1)[0] ?? ""
|
||||
const trimmed = firstLine.trim()
|
||||
// Must start with at least seven dashes (the separator) AND have
|
||||
// non-dash, non-whitespace content immediately after them on the
|
||||
// same line. A bare `-------` (separator on its own line that
|
||||
// happened to land in the search content for unrelated reasons) is
|
||||
// not flagged.
|
||||
const match = trimmed.match(/^-{7,}(.+)$/)
|
||||
if (!match) {
|
||||
return null
|
||||
}
|
||||
const tail = match[1].trim()
|
||||
// Allow lines that start with a literal `-` continuation (e.g.
|
||||
// markdown bullets that begin with extra dashes) — they wouldn't
|
||||
// look like the separator-then-code shape that confuses callers.
|
||||
if (tail === "" || /^-+$/.test(tail)) {
|
||||
return null
|
||||
}
|
||||
return firstLine
|
||||
}
|
||||
|
||||
function getSimilarity(original: string, search: string): number {
|
||||
// Empty searches are no longer supported
|
||||
if (search === "") {
|
||||
|
|
@ -321,6 +360,31 @@ export class MultiSearchReplaceDiffStrategy implements DiffStrategy {
|
|||
searchContent = this.unescapeMarkers(searchContent)
|
||||
replaceContent = this.unescapeMarkers(replaceContent)
|
||||
|
||||
// Detect the specific malformation flagged in #12210: an
|
||||
// unescaped `-------` separator that lacks the trailing
|
||||
// newline (e.g. `-------import { ... }`). The outer regex
|
||||
// makes the separator group optional, so the search content
|
||||
// silently absorbs the run-on line and matching fails with
|
||||
// a confusing "63% similar" error. Surface the actual root
|
||||
// cause so the model can self-correct.
|
||||
const malformedSeparator = detectMalformedSeparator(searchContent)
|
||||
if (malformedSeparator) {
|
||||
diffResults.push({
|
||||
success: false,
|
||||
error:
|
||||
`Malformed separator in SEARCH section\n\n` +
|
||||
`Debug Info:\n` +
|
||||
`- The "-------" separator that follows :start_line:/:end_line: must be on its own line, followed by a newline before the search content begins.\n` +
|
||||
`- Got: ${JSON.stringify(malformedSeparator)}\n` +
|
||||
`- Expected:\n` +
|
||||
` :start_line:7\n` +
|
||||
` -------\n` +
|
||||
` <search content here>\n` +
|
||||
`- Tip: insert a newline immediately after "-------". Do not put the first line of search content on the same line as the separator.`,
|
||||
})
|
||||
continue
|
||||
}
|
||||
|
||||
// Strip line numbers from search and replace content if every line starts with a line number
|
||||
const hasAllLineNumbers =
|
||||
(everyLineHasLineNumbers(searchContent) && everyLineHasLineNumbers(replaceContent)) ||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue