fix(opencode): prevent destructive edit matches (#30932)
This commit is contained in:
@@ -89,10 +89,14 @@ export const EditTool = Tool.define(
|
|||||||
Effect.gen(function* () {
|
Effect.gen(function* () {
|
||||||
if (params.oldString === "") {
|
if (params.oldString === "") {
|
||||||
const existed = yield* afs.existsSafe(filePath)
|
const existed = yield* afs.existsSafe(filePath)
|
||||||
const source = existed ? yield* Bom.readFile(afs, filePath) : { bom: false, text: "" }
|
if (existed) {
|
||||||
|
throw new Error(
|
||||||
|
"oldString cannot be empty when editing an existing file. Provide the exact text to replace, or use write for an intentional full-file replacement.",
|
||||||
|
)
|
||||||
|
}
|
||||||
const next = Bom.split(params.newString)
|
const next = Bom.split(params.newString)
|
||||||
const desiredBom = source.bom || next.bom
|
const desiredBom = next.bom
|
||||||
contentOld = source.text
|
contentOld = ""
|
||||||
contentNew = next.text
|
contentNew = next.text
|
||||||
diff = trimDiff(createTwoFilesPatch(filePath, filePath, contentOld, contentNew))
|
diff = trimDiff(createTwoFilesPatch(filePath, filePath, contentOld, contentNew))
|
||||||
yield* ctx.ask({
|
yield* ctx.ask({
|
||||||
@@ -111,7 +115,7 @@ export const EditTool = Tool.define(
|
|||||||
yield* events.publish(FileSystem.Event.Edited, { file: filePath })
|
yield* events.publish(FileSystem.Event.Edited, { file: filePath })
|
||||||
yield* events.publish(Watcher.Event.Updated, {
|
yield* events.publish(Watcher.Event.Updated, {
|
||||||
file: filePath,
|
file: filePath,
|
||||||
event: existed ? "change" : "add",
|
event: "add",
|
||||||
})
|
})
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -213,8 +217,8 @@ export const EditTool = Tool.define(
|
|||||||
export type Replacer = (content: string, find: string) => Generator<string, void, unknown>
|
export type Replacer = (content: string, find: string) => Generator<string, void, unknown>
|
||||||
|
|
||||||
// Similarity thresholds for block anchor fallback matching
|
// Similarity thresholds for block anchor fallback matching
|
||||||
const SINGLE_CANDIDATE_SIMILARITY_THRESHOLD = 0.0
|
const SINGLE_CANDIDATE_SIMILARITY_THRESHOLD = 0.65
|
||||||
const MULTIPLE_CANDIDATES_SIMILARITY_THRESHOLD = 0.3
|
const MULTIPLE_CANDIDATES_SIMILARITY_THRESHOLD = 0.65
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Levenshtein distance algorithm implementation
|
* Levenshtein distance algorithm implementation
|
||||||
@@ -296,6 +300,7 @@ export const BlockAnchorReplacer: Replacer = function* (content, find) {
|
|||||||
const firstLineSearch = searchLines[0].trim()
|
const firstLineSearch = searchLines[0].trim()
|
||||||
const lastLineSearch = searchLines[searchLines.length - 1].trim()
|
const lastLineSearch = searchLines[searchLines.length - 1].trim()
|
||||||
const searchBlockSize = searchLines.length
|
const searchBlockSize = searchLines.length
|
||||||
|
const maxLineDelta = Math.max(1, Math.floor(searchBlockSize * 0.25))
|
||||||
|
|
||||||
// Collect all candidate positions where both anchors match
|
// Collect all candidate positions where both anchors match
|
||||||
const candidates: Array<{ startLine: number; endLine: number }> = []
|
const candidates: Array<{ startLine: number; endLine: number }> = []
|
||||||
@@ -307,7 +312,10 @@ export const BlockAnchorReplacer: Replacer = function* (content, find) {
|
|||||||
// Look for the matching last line after this first line
|
// Look for the matching last line after this first line
|
||||||
for (let j = i + 2; j < originalLines.length; j++) {
|
for (let j = i + 2; j < originalLines.length; j++) {
|
||||||
if (originalLines[j].trim() === lastLineSearch) {
|
if (originalLines[j].trim() === lastLineSearch) {
|
||||||
candidates.push({ startLine: i, endLine: j })
|
const actualBlockSize = j - i + 1
|
||||||
|
if (Math.abs(actualBlockSize - searchBlockSize) <= maxLineDelta) {
|
||||||
|
candidates.push({ startLine: i, endLine: j })
|
||||||
|
}
|
||||||
break // Only match the first occurrence of the last line
|
break // Only match the first occurrence of the last line
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -324,7 +332,7 @@ export const BlockAnchorReplacer: Replacer = function* (content, find) {
|
|||||||
const actualBlockSize = endLine - startLine + 1
|
const actualBlockSize = endLine - startLine + 1
|
||||||
|
|
||||||
let similarity = 0
|
let similarity = 0
|
||||||
let linesToCheck = Math.min(searchBlockSize - 2, actualBlockSize - 2) // Middle lines only
|
const linesToCheck = Math.min(searchBlockSize - 2, actualBlockSize - 2) // Middle lines only
|
||||||
|
|
||||||
if (linesToCheck > 0) {
|
if (linesToCheck > 0) {
|
||||||
for (let j = 1; j < searchBlockSize - 1 && j < actualBlockSize - 1; j++) {
|
for (let j = 1; j < searchBlockSize - 1 && j < actualBlockSize - 1; j++) {
|
||||||
@@ -373,7 +381,7 @@ export const BlockAnchorReplacer: Replacer = function* (content, find) {
|
|||||||
const actualBlockSize = endLine - startLine + 1
|
const actualBlockSize = endLine - startLine + 1
|
||||||
|
|
||||||
let similarity = 0
|
let similarity = 0
|
||||||
let linesToCheck = Math.min(searchBlockSize - 2, actualBlockSize - 2) // Middle lines only
|
const linesToCheck = Math.min(searchBlockSize - 2, actualBlockSize - 2) // Middle lines only
|
||||||
|
|
||||||
if (linesToCheck > 0) {
|
if (linesToCheck > 0) {
|
||||||
for (let j = 1; j < searchBlockSize - 1 && j < actualBlockSize - 1; j++) {
|
for (let j = 1; j < searchBlockSize - 1 && j < actualBlockSize - 1; j++) {
|
||||||
@@ -675,6 +683,11 @@ export function replace(content: string, oldString: string, newString: string, r
|
|||||||
if (oldString === newString) {
|
if (oldString === newString) {
|
||||||
throw new Error("No changes to apply: oldString and newString are identical.")
|
throw new Error("No changes to apply: oldString and newString are identical.")
|
||||||
}
|
}
|
||||||
|
if (oldString === "") {
|
||||||
|
throw new Error(
|
||||||
|
"oldString cannot be empty when editing an existing file. Provide the exact text to replace, or use write for an intentional full-file replacement.",
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
let notFound = true
|
let notFound = true
|
||||||
|
|
||||||
@@ -693,6 +706,11 @@ export function replace(content: string, oldString: string, newString: string, r
|
|||||||
const index = content.indexOf(search)
|
const index = content.indexOf(search)
|
||||||
if (index === -1) continue
|
if (index === -1) continue
|
||||||
notFound = false
|
notFound = false
|
||||||
|
if (isDisproportionateMatch(search, oldString)) {
|
||||||
|
throw new Error(
|
||||||
|
"Refusing replacement because the matched span is much larger than oldString. Re-read the file and provide the full exact oldString for the intended replacement.",
|
||||||
|
)
|
||||||
|
}
|
||||||
if (replaceAll) {
|
if (replaceAll) {
|
||||||
return content.replaceAll(search, newString)
|
return content.replaceAll(search, newString)
|
||||||
}
|
}
|
||||||
@@ -709,3 +727,11 @@ export function replace(content: string, oldString: string, newString: string, r
|
|||||||
}
|
}
|
||||||
throw new Error("Found multiple matches for oldString. Provide more surrounding context to make the match unique.")
|
throw new Error("Found multiple matches for oldString. Provide more surrounding context to make the match unique.")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function isDisproportionateMatch(search: string, oldString: string) {
|
||||||
|
const oldLines = oldString.split("\n").length
|
||||||
|
const searchLines = search.split("\n").length
|
||||||
|
if (searchLines >= Math.max(oldLines + 3, oldLines * 2)) return true
|
||||||
|
if (oldLines === 1) return false
|
||||||
|
return search.trim().length > Math.max(oldString.trim().length + 500, oldString.trim().length * 4)
|
||||||
|
}
|
||||||
|
|||||||
@@ -106,21 +106,20 @@ describe("tool.edit", () => {
|
|||||||
}),
|
}),
|
||||||
)
|
)
|
||||||
|
|
||||||
it.instance("preserves BOM when oldString is empty on existing files", () =>
|
it.instance("rejects empty oldString on existing files and leaves content unchanged", () =>
|
||||||
Effect.gen(function* () {
|
Effect.gen(function* () {
|
||||||
const test = yield* TestInstance
|
const test = yield* TestInstance
|
||||||
const filepath = path.join(test.directory, "existing.cs")
|
const filepath = path.join(test.directory, "existing.cs")
|
||||||
const bom = String.fromCharCode(0xfeff)
|
const bom = String.fromCharCode(0xfeff)
|
||||||
yield* put(filepath, `${bom}using System;\n`)
|
const original = `${bom}using System;\n`
|
||||||
|
yield* put(filepath, original)
|
||||||
|
|
||||||
const result = yield* run({ filePath: filepath, oldString: "", newString: "using Up;\n" })
|
expect((yield* fail({ filePath: filepath, oldString: "", newString: "using Up;\n" })).message).toContain(
|
||||||
|
"oldString cannot be empty",
|
||||||
expect(result.metadata.diff).toContain("-using System;")
|
)
|
||||||
expect(result.metadata.diff).toContain("+using Up;")
|
|
||||||
|
|
||||||
const content = yield* loadRaw(filepath)
|
const content = yield* loadRaw(filepath)
|
||||||
expect(content.charCodeAt(0)).toBe(0xfeff)
|
expect(content).toBe(original)
|
||||||
expect(content.slice(1)).toBe("using Up;\n")
|
|
||||||
}),
|
}),
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -213,6 +212,53 @@ describe("tool.edit", () => {
|
|||||||
}),
|
}),
|
||||||
)
|
)
|
||||||
|
|
||||||
|
it.instance("rejects loose block-anchor matches and leaves content unchanged", () =>
|
||||||
|
Effect.gen(function* () {
|
||||||
|
const test = yield* TestInstance
|
||||||
|
const filepath = path.join(test.directory, "file.ts")
|
||||||
|
const original = [
|
||||||
|
"function configure() {",
|
||||||
|
" keepImportantState()",
|
||||||
|
" removeAllUserData()",
|
||||||
|
" archiveBackups()",
|
||||||
|
" auditLog()",
|
||||||
|
"}",
|
||||||
|
].join("\n")
|
||||||
|
yield* put(filepath, original)
|
||||||
|
|
||||||
|
expect(
|
||||||
|
(
|
||||||
|
yield* fail({
|
||||||
|
filePath: filepath,
|
||||||
|
oldString: ["function configure() {", " const enabled = true", "}"].join("\n"),
|
||||||
|
newString: ["function configure() {", " const enabled = false", "}"].join("\n"),
|
||||||
|
})
|
||||||
|
).message,
|
||||||
|
).toContain("Could not find oldString")
|
||||||
|
expect(yield* load(filepath)).toBe(original)
|
||||||
|
}),
|
||||||
|
)
|
||||||
|
|
||||||
|
it.instance("rejects block-anchor matches with unrelated middle content", () =>
|
||||||
|
Effect.gen(function* () {
|
||||||
|
const test = yield* TestInstance
|
||||||
|
const filepath = path.join(test.directory, "file.ts")
|
||||||
|
const original = ["function configure() {", " removeAllUserData()", "}"].join("\n")
|
||||||
|
yield* put(filepath, original)
|
||||||
|
|
||||||
|
expect(
|
||||||
|
(
|
||||||
|
yield* fail({
|
||||||
|
filePath: filepath,
|
||||||
|
oldString: ["function configure() {", " const enabled = true", "}"].join("\n"),
|
||||||
|
newString: ["function configure() {", " const enabled = false", "}"].join("\n"),
|
||||||
|
})
|
||||||
|
).message,
|
||||||
|
).toContain("Could not find oldString")
|
||||||
|
expect(yield* load(filepath)).toBe(original)
|
||||||
|
}),
|
||||||
|
)
|
||||||
|
|
||||||
it.instance("replaces all occurrences with replaceAll option", () =>
|
it.instance("replaces all occurrences with replaceAll option", () =>
|
||||||
Effect.gen(function* () {
|
Effect.gen(function* () {
|
||||||
const test = yield* TestInstance
|
const test = yield* TestInstance
|
||||||
|
|||||||
Reference in New Issue
Block a user